silverstripe / silverstripe-admin

Silverstripe Admin Component
BSD 3-Clause "New" or "Revised" License
25 stars 92 forks source link

Add natural sorting of menu title in CMSMenu::get_menu_items() #1399

Open patricknelson opened 1 year ago

patricknelson commented 1 year ago

Right now, menu items that happen to start with a lowercase letter (e.g. "eBay Earth") are unintuitively sorted to the bottom of the list of other modules of the same priority which all are using the typical uppercase letter. This may confuse users who are seeing modules clustered together alphabetically. It's also difficult for the developer to fix by manually setting a priority (or rearranging priority) every single time a new module is added. It's also difficult to fix for the developer due to the static references made to the CMSMain::get_menu_items() method where this is implemented (and within the class via self:: instead of static::), where typically the developer might use the Injector to override the class and then customize sorting on their own.

e.g. the current:

array_multisort($menuPriority, SORT_DESC, $menuTitle, SORT_ASC, $menuItems);

could instead be:

array_multisort($menuPriority, SORT_DESC, $menuTitle, SORT_NATURAL|SORT_FLAG_CASE, $menuItems);

Which will ignore case. Or, alternatively, we could make this an option where the default is to maintain the current SORT_ASC functionality with the user given the ability to override somehow by setting their own sorting flags.

michalkleiner commented 1 year ago

I'd be happy to look at a PR which adds the option to define the sorting via config. I think people are used to the semi-randomness of the items and if they changed order suddenly it could be more confusing than helpful for some.

patricknelson commented 1 year ago

If I have time 😄. Before I do though, quick questions on convention/code style:

  1. Since there's already a mix of constants and protected static variables, should private static variables go above or below those protected static declarations? Presumably still below the constants, right?
  2. What direction should we go in for sorting here? e.g. Should it be a simpler flag that you toggle on, or, should it be more nuts and bolts where you the sorting flags to use? Presumably all we need is more natural sort, but who knows... maybe someone will need something else in the future.

e.g.

// More approachable, but specialized. Just set to true to enable more natural sorting (ignore case)
private static $sort_ignore_case = false;
// vs. more direct and granular, but a little obscure. Set to SORT_NATURAL|SORT_FLAG_CASE to ignore case.
private static $sort_title_flags = SORT_ASC;
michalkleiner commented 1 year ago
  1. Not sure if any PSR ruleset dictates the order of class variables; I generally prefer private/protected/public order, below constants. But in this case I'd probably put it below the protected static variables since it feels like a lower priority.
  2. I'd probably use a single config holding whatever flags one wants to define, so $sort_title_flags. It looks like we can't use extension points as it's a static method and we'd need to add the Extensible trait as well. Was thinking to pass the ordered menu to an update method where one would be able to reorder it as much as you want.
patricknelson commented 1 year ago

Yeah, one issue I had with this was all the static calls being made to the class making it impossible to use the most intuitive technique for just extending/overriding the functionality myself (i.e. simply overriding ::get_menu_items()). So this was my alternative. An ->extend() might work as well.

One issue with passing flags might be that they are bitwise so you'd have to compute the bitwise value first before passing it into the YAML (unless there's some new fangled technique I'm not aware of available now).

For now I've just hacked something together that runs on LeftAndMain->init() (via existing extension) that just overrides the configs for all my admin modules' priority settings after a quick natural sort. At least for now until we find a suitable alternative.