-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
DropdownMenuV2
: rename to Menu
#66289
base: trunk
Are you sure you want to change the base?
Conversation
Size Change: -28 B (0%) Total Size: 1.81 MB
ℹ️ View Unchanged
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Flaky tests detected in 85c9ce1. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11444463017
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I generally agree with Menu
being a good pick with all the semantical and accessibility considerations, and the popover/dropdown wrapper being just a design detail, I'm wondering if you've considered the existing menu-like components (MenuGroup
and MenuItem
for example), which may start to feel confusing if we have a Menu
component group with subcomponents? Specifically, won't it be confusing to folks who want to create a non-dropdown menu (what MenuGroup
and MenuItem
do)?
I'd love to hear some feedback from @mirka and @DaniGuardiola on this one.
*/ | ||
import type { MenuContext as MenuContextType } from './types'; | ||
|
||
export const MenuContext = createContext< MenuContextType | undefined >( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is quite unfortunate that due to the relatively big number of line changes, Git doesn't recognize this as a renamed file but rather as a new one. This will, unfortunately, cause it to lose all prior history.
Would you be willing to do a directory rename separately from the actual changes inside the files? That could help us keep all the prior history.
What?
Rename the experimental (private)
DropdownMenuV2
component toMenu
Why?
The "dropdown" part of the name implied that the component can only be displayed as a menu inside of a popover, activated by a trigger.
For the sake of reusability, we don't want to preclude the possibility that the component can offer a "menu" primitive (eg. "menu list") that doesn't necessarily render inside a popover.
The "Menu" name also better aligns with the WAI-ARIA "menu" pattern and with how ariakit named its equivalent component.
How?
Find and replace.
Testing Instructions
Smoke test Storybook and the editor, make sure that all usages of the component work as expected.