Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate DropdownMenu instances to Menu #61094

Open
19 tasks
jameskoster opened this issue Apr 25, 2024 · 5 comments
Open
19 tasks

Migrate DropdownMenu instances to Menu #61094

jameskoster opened this issue Apr 25, 2024 · 5 comments
Labels
[Feature] UI Components Impacts or related to the UI component system General Interface Parts of the UI which don't fall neatly under other labels. [Type] Enhancement A suggestion for improvement.

Comments

@jameskoster
Copy link
Contributor

jameskoster commented Apr 25, 2024

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

  • Block options menus (appears in block toolbar and List View) Migrate block options menu to DropdownMenu v2 and use flyout menus to re-organise actions #49271
  • Block toolbar menus, e.g. text alignment
  • Editor modes
  • Inserter -> Pattern filter menu
  • View menu in top toolbar
  • Options menu in top toolbar
  • Document actions menu in Inspector
  • Toolspanel ellipsis menu
  • Ellipsis menu in global styles panel header
  • Block transforms
  • Navigation menu switcher/tools
  • Media replacement (see Site Logo block inspector/toolbar)
  • Aspect ratio (in block toolbar when cropping an image)
  • Template menu in page details
  • Quick Inserter

Admin

  • Add pattern menu
  • Navigation menu actions (in details panel)
  • Template actions (in details panel)
  • Template part actions (in details panel)

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 #57996.

@jameskoster jameskoster added [Type] Enhancement A suggestion for improvement. General Interface Parts of the UI which don't fall neatly under other labels. [Feature] UI Components Impacts or related to the UI component system labels Apr 25, 2024
@jameskoster
Copy link
Contributor Author

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:

  • 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?
  • Should we utilise the ellipsis suffix on menu items that have subsequent steps, e.g. "Rename..." vs "Rename"?
  • Can we get away without group headings?
  • Are there design details in DropdownMenu V2 that need to change?

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

@tyxla
Copy link
Member

tyxla commented Jun 18, 2024

cc @ciampo who's been working on DropdownMenuV2

@ciampo
Copy link
Contributor

ciampo commented Jun 19, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

ciampo commented Jul 4, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system General Interface Parts of the UI which don't fall neatly under other labels. [Type] Enhancement A suggestion for improvement.
Projects
Status: No status
Development

No branches or pull requests

3 participants