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

Components: reconcile ItemGroup/Item with MenuGroup/MenuItem #35210

Open
3 tasks
Tracked by #34709
ciampo opened this issue Sep 29, 2021 · 2 comments
Open
3 tasks
Tracked by #34709

Components: reconcile ItemGroup/Item with MenuGroup/MenuItem #35210

ciampo opened this issue Sep 29, 2021 · 2 comments
Labels
[Feature] Component System WordPress component system [Package] Components /packages/components [Type] Task Issues or PRs that have been broken down into an individual action to take

Comments

@ciampo
Copy link
Contributor

ciampo commented Sep 29, 2021

What

This conversation was started in #35142 (comment)

We should look into refactoring MenuGroup and MenuItem to rely on the lower level ItemGroup and Item. This would enable the usage of MenuGroup and MenuItem outside of DropdownMenu and would give us a chance to enforce a more consistent look&feel (including spacing) across menu layouts.

Why

The MenuGroup and MenuItem components were created as high-level components, tailored to be used inside the DropdownMenu component. As such, they are quite opinionated in terms of:

  • the component that MenuItem renders (which is always a Button)
  • the internal layout of each MenuItem
  • the aria roles (DropdownMenu is supposed to have role="menu", MenuGroup would have role="group" and MenuItem the role="menuitem")
  • and more.

On the other hand, the recently introduced ItemGroup and Item components offer a more low-level approach, are much less opinionated and rely on composition with other components. Their low-level approach can require a lot of tweaking when used directly in complex UIs, and therefore can easily lead to inconsistencies when used in different parts of the editor.

That's why we should look into uniforming the look&feel of menus across different UI patterns (dropdown menus, navigation menus, modal menus, sidebar menus...) and consider rewriting MenuGroup and MenuItem using ItemGroup and Item, with the intention of unlocking their usage in more situations outside of DropdownMenu.

A/C

  • Find a way to ensure consistent spacing for all menus using both ItemGroup/Item and MenuGroup/MenuItem, matching the spacing of other existing components (e.g. aligning with the icons in the Modal component).
  • Convert MenuGroup and MenuItem to TypeScript, switch to the new styling system, and hook to the Context System
  • Rewrite MenuGroup and MenuItem to use internally ItemGroup and Item and expose more flexibility
@ciampo ciampo added [Feature] Component System WordPress component system [Package] Components /packages/components [Type] Enhancement A suggestion for improvement. [Type] Task Issues or PRs that have been broken down into an individual action to take and removed [Type] Enhancement A suggestion for improvement. labels Sep 29, 2021
@ciampo ciampo assigned ciampo and unassigned ciampo Sep 29, 2021
@youknowriad
Copy link
Contributor

We should also consider semantics/a11y. What makes most sense for these components. Should we have a new Menu wrapper (role="menu"). Should the low level components stay without any built-in semantics...

@ciampo
Copy link
Contributor Author

ciampo commented Sep 29, 2021

Currently, ItemGroup has a default role="list" and Item has a default role="listitem", which I believe are good defaults — although they can be both easily overridden.

We could also use the recently introduced Context system to suggest the correct roles (in case they are not explicitly set) — e.g., DropdownMenu could use the Context System to suggest to its child ItemGroup and Item components to have role="group" and role="menuitem".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Component System WordPress component system [Package] Components /packages/components [Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

No branches or pull requests

2 participants