extract and standardize how we show key-value pairs in lists in RT's UI.

[obra]

This PR adds a new "LabelValueRow" component to extract and standardize how we show key-value pairs in lists in RT's UI.

This makes it easier to keep the style consistent, easier to update the style later and reduces boilerplate html in code.

Ideally, this PR would result in zero user-visible changes within RT.

As a first step, I've updated Ticket/Elements/ShowBasics to use the new component.

If y'all believe this would be desirable change, I'm happy to update the entire codebase to use the new component, either as one PR or as a series of smaller PRs.

Since it's going to touch a lot of the UI, if you want the updates all together, I should probably do those updates in one pass in advance of a relatively quick merge or there will be numerous merge conflicts.

If you want me to keep going, please let me know which branch I should be targeting and whether you have stylistic preferences.

This replaces #339

[cbrandtbuffalo]

Looks ok to me and makes sense. I would also add args for the col-3, col-9 classes. Default to col-3/9, but allow override. I think we have some cases with different values, like for transaction CFs. And there might be something in assets.

If you wanted to do it everywhere, I would be ok to merge since, as you noted, it should result in no visible changes.

[sunnavy]

I like the idea. 2 suggestions:

1. We use CamelCase argument names in most mason templates, maybe stick to that for consistency?

BTW, the 2 additional args Jim mentioned are $LabelCols and $ValueCols, which are used in other mason code like /Elements/ShowCustomFields

2. Maybe make LabelRowValue support filtering too?

With it, we won't need to create things like "ShowTimeWorkedByUser", we can insert content directly like:

<&| /Elements/LabelRowValue, ... &>
    # the content of ShowTimeWorkedByUser
</&>

[obra]

I'm 100% happy to fix the stylistic issues.

I'll look at supporting filtering (although I do think there's some value to breaking out complicated bits of rendering into components.)

I explicitly don't want to have calling code pass in hardcoded width values because those would force more brittleness into the layout engine that I'm pretty sure can be avoided.

Further down the line, if we wanted to, say, have views that used a more compact pattern of sticking the label above the value in smaller text, having the calling code all assuming a specific old-school grid model would make life harder.

Does that make sense?

[cbrandtbuffalo]

I explicitly don't want to have calling code pass in hardcoded width values because those would force more brittleness into the layout engine that I'm pretty sure can be avoided.

Further down the line, if we wanted to, say, have views that used a more compact pattern of sticking the label above the value in smaller text, having the calling code all assuming a specific old-school grid model would make life harder.

Does that make sense?

Yeah, if the new template includes the classes enabling the grid context (form-row), that makes sense. We can keep the current bootstrap code for areas that want different col sizes.

I do think tweaking the 3/9 numbers could be a desired customization in the grid model, so a hook to change that would make the new template more helpful. Maybe change to $LabelClass and $ValuesClass to avoid the specific "cols"? Or maybe a callback to set/modify those?

[obra]

One thing I'm trying to do with this is to start to decouple the bootstrap-specific presentation details from the logical components. (To that end, in my local branch, I've renamed the component from "LabelValueRow" to "LabeledValue")

What got me headed in this direction was Keri mentioning that y'all might want to be able to put ticket metadata in a column next to the history.

Lifting out "show a value with a label" into its own component means that the display change necessary to go from this:

to this:

is about two lines of markup if one wants to make that change. It should be possible to toggle the rendering style between the different versions without changing every template showing a labeled value.

Right now, I'm at 76 files changed, 1048 insertions(+), 2525 deletions(-) on my local branch and am not 100% done, but approaching being ready for a first review.

Once this one is put to bed, the two other similar-ish cleanups I want to propose are to the wrapper around 99% of the /Elements/Submit calls in the codebase and extracting out the boilerplate for modals into a component.

[obra]

@sunnavy - Making it a Component With Content was brilliant. Thank you.

I've ported the vast majority of RT's labeled values to the new LabeledValue component.

At this point, I'd love feedback and code review.

Since it touches so many files, I think it makes sense to work to get this or some variant possibly merged before trying to apply it to knottier bits of the codebase and to handle those as smaller PRs, but I'm open to whatever works best for you folks.

[obra]

@cbrandtbuffalo @sunnavy - when you can, let me know what changes you'd like to this PR. I think the right thing to do is to be targeting whatever the next stable RT is rather than the current stable branch.

[cbrandtbuffalo]

@cbrandtbuffalo @sunnavy - when you can, let me know what changes you'd like to this PR. I think the right thing to do is to be targeting whatever the next stable RT is rather than the current stable branch.

We discussed and the downside of branching from master is that after the merge, merging up from 5.0-trunk is going to get more difficult. As long as the changes don't cause big feature shifts on 5.0, we're good with branching from 5.0-trunk. That will also allow us to get eyes on it sooner since we can start testing soon after it gets merged.

Open to other thoughts if you see any risk to going with 5.0-trunk or if you think the changes might not qualify as "stable".

[obra]

i’m okay with it going on 5.0-trunk. there are some bigger cleanups that I would want to do that I don’t think makes sense for 5.0-trunk and I’m not sure how you would want to handle those.

On Aug 19, 2022, at 8:43 AM, Jim Brandt @.***> wrote:

 @cbrandtbuffalo @sunnavy - when you can, let me know what changes you'd like to this PR. I think the right thing to do is to be targeting whatever the next stable RT is rather than the current stable branch.

We discussed and the downside of branching from master is that after the merge, merging up from 5.0-trunk is going to get more difficult. As long as the changes don't cause big feature shifts on 5.0, we're good with branching from 5.0-trunk. That will also allow us to get eyes on it sooner since we can start testing soon after it gets merged.

Open to other thoughts if you see any risk to going with 5.0-trunk or if you think the changes might not qualify as "stable".

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

[cbrandtbuffalo]

i’m okay with it going on 5.0-trunk. there are some bigger cleanups that I would want to do that I don’t think makes sense for 5.0-trunk and I’m not sure how you would want to handle those.

Could you do the cleanups in a separate branch from master? Or are they things you would want to do along with the other changes?

[obra]

I guess my real question is where one should be targeting not-backwards-compatible changes for 6.0?

On Fri, Aug 19, 2022 at 10:34 AM Jim Brandt @.***> wrote:

i’m okay with it going on 5.0-trunk. there are some bigger cleanups that I would want to do that I don’t think makes sense for 5.0-trunk and I’m not sure how you would want to handle those.

Could you do the cleanups in a separate branch from master? Or are they things you would want to do along with the other changes?

— Reply to this email directly, view it on GitHub https://github.com/bestpractical/rt/pull/341#issuecomment-1220923269, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAALC2CE57IYLLSUT6SE3ZLVZ7ATXANCNFSM55IKIWJA . You are receiving this because you authored the thread.Message ID: @.***>

[cbrandtbuffalo]

I guess my real question is where one should be targeting not-backwards-compatible changes for 6.0?

master is the starting point for for the next release, so you can branch from there.

[obra]

Got it. once this pr is merged to 5.0-trunk and merged down to master, I’ll target bigger changes over there

On Aug 19, 2022, at 1:57 PM, Jim Brandt @.***> wrote:

 I guess my real question is where one should be targeting not-backwards-compatible changes for 6.0?

master is the starting point for for the next release, so you can branch from there.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

[obra]

Once this is in, it should become easier to add aria labeling to form values more systematically.

[cbrandtbuffalo]

I made some fixes from the initial PR and rebased on top of 5.0-trunk and pushed that here:

https://github.com/bestpractical/rt/tree/5.0/extract-label-value-rows

If you could start from there with any new changes, that will pick up those fixes.

A few other areas where something is off:

• Admin > Tools > Shredder: Behavior is kind of strange, some plugin sections don't load for me when selecting different values from Select Plugin (behavior is inconsistent). I got Attachments to load and it was skewed a bit.
• The Search > Assets > Simple Search page, Basics section, has some fields not lining up
• Something on ticket display causes the columns to shift. I'm guessing it's in More about requestors as it seems to happen only if there is a requestor who isn't the logged in user
• On ticket create, on the Details tab, Links ends up at the bottom

[obra]

Rebased on top of that branch. I've fixed (as far as I can tell) everything except the ticket display rendering issue you've flagged. I can't replicate that one. Can you tell me a bit more about it?

[obra]

And ModifyScrip. That one is incoming.

[obra]

I pushed up the change to fix 'Applies To' on scrip edit to set it back to 3/9.

I think that custom widths for any given admin form are probably not the right optimization. We have enough similar list-of-stuff forms that we'd be better off solving it systematically.

[cbrandtbuffalo]

I pushed up the change to fix 'Applies To' on scrip edit to set it back to 3/9.

I think that custom widths for any given admin form are probably not the right optimization. We have enough similar list-of-stuff forms that we'd be better off solving it systematically.

Agree.

Rebased on top of that branch. I've fixed (as far as I can tell) everything except the ticket display rendering issue you've flagged. I can't replicate that one. Can you tell me a bit more about it?

I think it might be related to custom roles in the People section. In my test case, I had a custom role that sorts right after Owner. Applying that to a queue seems to trigger the issue, which also shows up as the People portlet rendering in edit mode.

I noticed the footer has a white gap without the custom role applied, so maybe a missing closing div or something that causes different issues if the custom role is or isn't there.

[obra]

Looks like the issue may have been in the Links rendering. I've cleaned it up. I think this is ready for re-review.

On Thu, Aug 25, 2022 at 6:55 AM Jim Brandt @.***> wrote:

I pushed up the change to fix 'Applies To' on scrip edit to set it back to 3/9.

I think that custom widths for any given admin form are probably not the right optimization. We have enough similar list-of-stuff forms that we'd be better off solving it systematically.

Agree.

Rebased on top of that branch. I've fixed (as far as I can tell) everything except the ticket display rendering issue you've flagged. I can't replicate that one. Can you tell me a bit more about it?

I think it might be related to custom roles in the People section. In my test case, I had a custom role that sorts right after Owner. Applying that to a queue seems to trigger the issue, which also shows up as the People portlet rendering in edit mode.

I noticed the footer has a white gap without the custom role applied, so maybe a missing closing div or something that causes different issues if the custom role is or isn't there.

[image: Screen Shot 2022-08-25 at 9 38 10 AM] https://user-images.githubusercontent.com/1554082/186682358-897b9ecd-a6c1-46fd-b6f6-273b4518a20c.png [image: Screen Shot 2022-08-25 at 9 39 04 AM] https://user-images.githubusercontent.com/1554082/186682361-6b83db62-b4c6-4ad6-b0b4-f1728cbd77a1.png

— Reply to this email directly, view it on GitHub https://github.com/bestpractical/rt/pull/341#issuecomment-1227296515, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAALC2EV5FB55WUQ6WJWNZTV253MTANCNFSM55IKIWJA . You are receiving this because you authored the thread.Message ID: @.***>

[obra]

Good catch. Thank you.

On Fri, Sep 2, 2022 at 12:13 PM Jim Brandt @.***> wrote:

@.**** commented on this pull request.

Comment added, will take care of it when merging.

— Reply to this email directly, view it on GitHub https://github.com/bestpractical/rt/pull/341#pullrequestreview-1095255794, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAALC2FAHJRSNW4KSCZPDBLV4JGWXANCNFSM55IKIWJA . You are receiving this because you authored the thread.Message ID: @.***>

[cbrandtbuffalo]

Merged into 5.0-trunk. Thanks!