I'm not sure why we never got round to doing this, I think I remember this coming up a few times already: properties in the "Add fields" form (modal) are currently unsorted, which is pretty confusing and makes it hard to find anything.
Adding a sort (alphabetical sort on the label, I'd say) would be a very simple way to improve UX there, I think.

CommentFileSizeAuthor
#9 2881631-9--sort_add_fields_properties.patch1.67 KBdrunken monkey
#9 2881631-9--sort_add_fields_properties--interdiff.txt3.13 KBdrunken monkey
#8 2881631-8.patch1.59 KBjzavrl
#7 2881631-7.patch1.59 KBjzavrl
#3 2881631-3.patch2.01 KBjzavrl

Comments

drunken monkey created an issue. See original summary.

jzavrl’s picture

jzavrl commented
Assigned: Unassigned » jzavrl

Just did something similar regarding UX on the add fields page, can take a look at this as well.

jzavrl’s picture

jzavrl commented
Assigned: jzavrl » Unassigned
Status: Active » Needs review
StatusFileSize
new 2881631-3.patch2.01 KB

Here we go. Are tests necessary for this kind of a cosmetic update? The only way I can think of testing this is with unit tests, run the same uasort function and checking against the original array. If that would be necessary that is.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Needs work

I don't think we need tests. But I also don't see the need to add this to the interface. That only adds overhead. Let's make that method protected instead.

jzavrl’s picture

jzavrl commented

Do you mean completely remove the uasort callback from the FieldsHelper or just from the interface?

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

I'd suggest removing it from the interface, but maybe @drunken monkey feels differently.

jzavrl’s picture

jzavrl commented
StatusFileSize
new 2881631-7.patch1.59 KB

Here is a patch without the interface addition. I do think however, it should be in the FieldsHelper service. Which would also mean to leave the method as public.

jzavrl’s picture

jzavrl commented
Status: Needs work » Needs review
StatusFileSize
new 2881631-8.patch1.59 KB

Reattached, to run the tests again (just to be sure).

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
StatusFileSize
new 2881631-9--sort_add_fields_properties--interdiff.txt3.13 KB
new 2881631-9--sort_add_fields_properties.patch1.67 KB

Thanks a lot for the patch!
However, there are several problem with it, or at least things I'd prefer differently:

  • Actual problem: Your patch would only sort the uppermost layer of properties, not the nested properties.
  • The sort method is much too specialized (checking $a['label']['#markup']) to be included in a service. Let's just keep it on the form class.
  • If we only set #theme after sorting, we don't need those safety checks for unexpected structures being passed, but can just rely on the two compared items looking as they should.

Revised patch attached, please test/review!

In any case, I agree, we don't need tests for that.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

This looks good, makes it a lot slimmer. Would love to have @jzavrl confirm that this also works.

jzavrl’s picture

jzavrl commented

You're right, I forgot about the nested properties. Patch looks good, confirming RTBC.

  • drunken monkey committed b471840 on 8.x-1.x authored by jzavrl 
    Issue #2881631 by jzavrl, drunken monkey, borisson_: Added alphabetic...
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Reviewed & tested by the community » Fixed

Thanks for the review and the confirmation, good to hear!
Committed.
Thanks again, everyone!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.