ToolsPanel: Make menu item order consistent for SlotFill use cases

[aaronrobertshaw]

Fixes: #49124

What?

Updates the ToolsPanel to enforce some menu item order consistency when fills from different sources are rerendering.

Why?

Watching the menu item position jump around when hovering over the padding or margin controls in the Dimensions panel, and again when resetting that panel's items, is not a great experience.

How?

• Adds some internal state to track the order items first register
• The above initial order is used to maintain the ordering of menu items

Alternative Solutions

We (@glendaviesnz and I) looked at ways of preventing the on-hover behaviour of the spacing controls from causing additional rerenders which result in the altered menu item order but didn't have much success in finding a clean solution.

Ultimately, if we can prevent the inconsistent menu order at the component level it might help protect against the same issue with other panels as their use cases evolve. Happy to explore better approaches if anyone has ideas.

To that end, I explored using the insertion order of properties within the currentMenuItems argument to generateMenuItems. That wasn't successful when switching or clearing block selection then re-selecting the original cover block.

Testing Instructions

1. On trunk, follow the replication steps as per the original issue
2. Checkout this PR and repeat the process confirming the menu item order remains consistent
3. Test other panels in the editor and storybook examples and confirm no regressions in behaviour
4. Run the unit tests: npm run test:unit packages/components/src/tools-panel

Screenshots or screencast

Before	After
Screen.Recording.2023-03-21.at.5.36.12.pm.mp4	Screen.Recording.2023-03-21.at.5.37.31.pm.mp4

[aaronrobertshaw]

@ciampo or @mirka do you any wisdom to share on better approaches to maintaining the order of a ToolsPanel's menu items?

[ciampo]

Hey 👋 Thanks for the ping!

What is the expected behaviour on paper?

Probably the cleaner approach would be for the component to have a defined rule for knowing the sorting order in a "stateless" way — ie. having a rule/algorithm that, when be applied on any re-render, yields the same result (e.g some kind of alphabetical order). Such an approach would limit the amount of extra code/state in a component that is already quite complex.

Of course, in case that panels should be sorted based on the order they first registered themselves, some amount of state seems necessary

[glendaviesnz]

This tested well for me.

ie. if there's a rule algorithm that, when be applied on any re-render, yields the same result (e.g some kind of alphabetical order)

I think it makes sense that the menu is ordered in the same way as the items appear in the panel, seems like it would appear a little odd if min-height was at the bottom of the panel, but the middle menu option if it was an alpha sort?

[ciampo]

I think it makes sense that the menu is ordered in the same way as the items appear in the panel, seems like it would appear a little odd if min-height was at the bottom of the panel, but the middle menu option if it was an alpha sort?

Absolutely — whatever sorting we pick, it should be applied to both the ToolsPanelItems and the menu items.

[aaronrobertshaw]

Thanks for the feedback @ciampo and @glendaviesnz 👍

Probably the cleaner approach would be for the component to have a defined rule for knowing the sorting order in a "stateless" way — ie. having a rule/algorithm that, when be applied on any re-render, yields the same result (e.g some kind of alphabetical order).

The problem as I see it is that we can't guarantee the order of fills within a slot. My understanding is they have a slightly different life cycle and so we can also end up with some stale or orphaned fills, hence the filtering of them by panelId.

To be able to match panel designs we can't rely on an arbitrary alphabetical rule if we tie the order of the displayed controls within the order of the menu items. The current approach was the best I could think of short of implementing an explicit ordering mechanism. It, unfortunately, is not stateless though.

I think it makes sense that the menu is ordered in the same way as the items appear in the panel, seems like it would appear a little odd if min-height was at the bottom of the panel, but the middle menu option if it was an alpha sort?

Agreed. Is that not the case here now though? Excepting the split between default and optional controls of course.

We can't guarantee the position of fills in a slot. We get around that for the display of controls by rendering a placeholder div in the DOM so that its position is maintained when toggled on and off. That DOM order should be the order in which the fills were initially rendered/registered.

Absolutely — whatever sorting we pick, it should be applied to both the ToolsPanelItems and the menu items.

The current approach in this PR could be viewed as applied to both the ToolsPanelItems (via the placeholder divs), and the menu items (via the registration order state).

I'm sort of out of ideas at this point to fix the regression caused in #44955. Perhaps @glendaviesnz can address the re-rendering resulting from the useVisualizer hook in the Dimensions panel to buy us time to settle on a catch-all solution in the ToolsPanel?

[glendaviesnz]

Agreed. Is that not the case here now though?

Yeh, sorry for the confusion, that is the case in this PR - was noting that it wouldn't be if the menu items were sorted in a way that didn't match the manual placement of the controls.

This is already a pretty complex component and I don't think this addition makes it that much more complex, and in fact, I think it is now easier to grok how the ordering of the menu items is happening.

If we just adjust the rerendering of the spacing inputs this is guaranteed to come back and bight us with another component so my suggestion would be to go with this approach for now as I can't see an obvious alternative right now in terms of tying the fixed sorting of the controls to the menu items.

[ciampo]

Thank you both for replying. If you're both confident that there isn't clearly a simpler/better approach, I'm happy to go with this one.

[glendaviesnz]

This tested well for me:

✅ Tests passed
✅ Menu items sort order remained consistent across all workflows, including when mousing over spacer preset controls
✅ The reset/reset all options all still worked as expected with the menus

[aaronrobertshaw]

If you're both confident that there isn't clearly a simpler/better approach, I'm happy to go with this one.

I can't think of any at this stage. If any do present themselves, I'll happily implement them.

This tested well for me:

👍 Thanks for the review and testing @glendaviesnz, I'll get this merged.