search

WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.57k stars 4.22k forks source link

Migrate DropdownMenu instances to Menu #61094

Open jameskoster opened 7 months ago

jameskoster commented 7 months ago

Menu (formerly 'DropdownMenu V2') includes several benefits like support for flyout menus, improved accessibility, and consistent treatment of checkbox/radio groups. It would be good to migrate instances of DropdownMenu to the new component to make use of those benefits, and visually align menus across.

Editor

Admin

Many of these menus can be replicated 1:1 with the new component, so hopefully a lot of the work will be straight-forward. Some will however require some design attention. As time allows let's mock these up in Figma here.

Some initial exploration around this took place in https://github.com/WordPress/gutenberg/pull/57996.

jameskoster commented 7 months ago

I've mocked up the menus in Figma based on the existing DropdownMenu V2 component.

Here's a sample (current menus are on the left):

Export

Generally I think the migration should be quite smooth, but some questions arose out of this exercise:

Also worth noting: Transforms and Toolspanel will need further investigation as these are more involved menus.

tyxla commented 5 months ago

cc @ciampo who's been working on DropdownMenuV2

ciampo commented 5 months ago

Hey @jameskoster šŸ‘‹ it's good to be back!

  • Do we need a version of DropdownRadioItem where the radio indicator is an icon? If yes, how do we differentiate that from regular menu items with icons, and how do we mark the selected item?

We discussed this aspect a few months ago and concluded that radio items shouldn't show an icon. Have there been any significant pushbacks on this decision or any other reasons why we are reconsidering it?

  • Should we utilise the ellipsis suffix on menu items that have subsequent steps, e.g. "Rename..." vs "Rename"?

Are you proposing that the component automatically adds the ellipsis to the menu item label when it is a sub-menu trigger? Although in the screenshot above, "Rename..." and "Duplicate..." are probably triggering a modal dialog rather than a submenu.

IMO, whether to add the ellipsis to the menu item label or not shouldn't be a responsibility of the DropdownMenu component ā€” also because the component in itself doesn't have a way to know when a menu item will open a modal dialog vs only perform an action.

  • Can we get away without group headings?

I think we need them unless we want to make an explicit design decision and move away from them. Group headings would be easy to implement, in case.

  • Are there design details in DropdownMenu V2 that need to change?

I think that the current implementation is already quite solid ā€” unless there are big aspects that need change, I think that the best way forward is to continue to work on replacing existing v1 usages with v2, which should help us uncover any major bugs / missing features and should therefore also inform any design tweaks.

In January, before I took some extended leave, I started refactoring the "More" dropdown in the block toolbar, which is one of the most complex use cases and should give us a good taste for the rest of the migration. I plan on resuming that work soon

jameskoster commented 5 months ago

We discussed this aspect a few months ago and concluded that radio items shouldn't show an icon. Have there been any significant pushbacks on this decision or any other reasons why we are reconsidering it?

No significant pushback that I've seen, and I still think the overall consistency is worth the trade-off of losing the icon for radio items. But this is just my opinion :) We may see some pushback when updating menus like text justification in the block toolbar.

Are you proposing that the component automatically adds the ellipsis to the menu item label when it is a sub-menu trigger?

The rule of thumb would be; if the user must take subsequent action after clicking, then the label would include an ellipsis. It's becoming a more established convention that we might consider adopting.

Screenshot 2024-06-20 at 10 50 29

One way we might make this part of the component would be to add a hasNextStep (or similar) prop, so that we can update holistically in the future. It's probably a separate consideration from this work.

I think we need them unless we want to make an explicit design decision and move away from them. Group headings would be easy to implement, in case.

I think the main consideration here is the a11y story. I haven't checked in a while, but I don't think headings should live inside role="menu" elements, so we'd need to do something like:

<h2 id="menuHeading">Menu Title</h2>
<ul role="menu" aria-labelledby="menuHeading">
  <li role="menuitem">Item 1</li>
  <li role="menuitem">Item 2</li>
  <li role="menuitem">Item 3</li>
</ul>
ciampo commented 5 months ago

I still think the overall consistency is worth the trade-off of losing the icon for radio items. But this is just my opinion :) We may see some pushback when updating menus like text justification in the block toolbar.

That is my opinion too. Let's see if we can get away with using regular menu items (instead of radio) for those instances, which would avoid the issue altogether.

if the user must take subsequent action after clicking, then the label would include an ellipsis [...] One way we might make this part of the component would be to add a hasNextStep (or similar) prop, so that we can update holistically in the future. It's probably a separate consideration from this work.

Thank you for clarifying! Personally, I still think that adding ellipses should not be a responsibility of DropdownMenu. Components should focus on the core features that they offer, and should try to do that in the most general and open way possible ā€” this is to allow flexibility and composition with other components. That is why, for example, we have a prefix prop instead of icon and a suffix prop instead of shortcut from the legacy V1.

Although I agree that it's still a separate conversation, so let's resume it if and when necessary.

think the main consideration here is the a11y story. I haven't checked in a while, but I don't think headings should live inside role="menu" elements, so we'd need to do something like:

That shouldn't be a problem. ariakit already has the primitives that we'd need ā€” MenuGroup and MenuGroupLabel

Screenshot 2024-07-04 at 12 10 31