Components: Add Menu #29645
Components: Add Menu #29645
Conversation
sarayourfriend
added
[Package] Components
sarayourfriend
requested review from
ajitbohra and
chrisvanpatten
as code owners
|
Size Change: +2.54 kB (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for working on this @sarayourfriend! ❤️
What's the reason for the fallback to View when there's no menu prop? In which case menu will exist and not?
Is this component always intended to be used as a dropdown menu (with a menu button that opens a popover)? I ask this because Reakit's Menu component can only be used like that. Maybe that's the reason for the fallback?
| * @param {import('react').Ref<any>} forwardedRef | ||
| */ | ||
| function Menu( props, forwardedRef ) { | ||
| const { children, className, menu, ...otherProps } = useContextSystem( |
There was a problem hiding this comment.
It looks like this menu is undefined, so View is used instead of ReakitMenu. View isn't a tabbable element. That's why it's not accessible with the keyboard.
How can we inject menu (which I believe would come from useMenuState, right?) into the context system?
| ); | ||
|
|
||
| const resizeListener = usePopoverResizeUpdater( { | ||
| onResize: menu?.unstable_update, |
There was a problem hiding this comment.
If we're going to use unstable features from packages, we should make sure we use the exact version for that package on package.json so as to avoid issues like #23332.
I'm not sure, @ItsJonQ will have to answer that, but I think your hunch about that being the reason for the fallback makes sense as there is a separate |
sarayourfriend
force-pushed
the
add/hooks-heading
branch
from
0250ebc to
672ebd6
Compare
Haiii!! @sarayourfriend + @diegohaz I can provide some context :). I created the But it sounds like the Reakit menu is intentionally designed to only work within a If so, we may need to create a dedicated component for Sidebar/navigation based menus 🤔 |
I think we could solve this by adding tab indexes right? Or are there specific aria roles/attributes we need to apply to a sidebar type menu? |
I think it's just a tab index change. @diegohaz What do you think :). Oh! It's worth noting that I previously experienced focus/scroll jumping when I used the Reakit |
A menu that's always visible is a
But since we're talking about adding tab indexes, I guess we should use no ARIA in this case. For a static list of actions, just use a static list element. |
sarayourfriend
force-pushed
the
add/hooks-menu
branch
from
27aa28e to
6151d81
Compare
I think we should just render a |
You know I thought we were, and indeed we do render a |
For your (👇 Scroll to bottom for TLDR) Original DesignThis discussion and breakdown was really insightful for me.
Even with that in mind, I believe I may have unnecessarily tangled the implementation - that being making Menu "aware" of Dropdown (which @sarayourfriend untangled here). Naming ConventionsThis PR nudged me to look more into the name "Dropdown" vs. "Menu". Unfortunately, it's still unclear 😅 .
In the case of Gutenberg and With all that said... TLDRUltimately, we need 2 components:
We may re-use the Menu UI within Dropdown (and friends), but we don't have to. I think this PR provides us with an opportunity to intentionally define how we want these base level components to be :) (Doesn't have to be in this PR) Hope this helps! Love to hear your thoughts ❤️ |
|
I like your summary of the state of things @ItsJonQ. Worst case we can employ these component's polymorphism to reuse them if desired without muddling up their internals. |
|
We already have standard Even though these behaviors are not required, I don't think we should use these roles in a static list with tabbable elements like this component, which would be better described as Using the names On the API aspect, I prefer separate components for different semantics. For example, instead of Alternatively, I've seen other libraries exposing very generic components like <List>
<Item />
<Item />
<Item />
</List><ActionList>
<Item />
<Item />
<Item />
</ActionList><Menu>
<Item />
<Item />
<Item />
</Menu><DropdownMenu>
<Item />
<Item />
<Item />
</DropdownMenu>But, instead of |
|
Major +1 to Diego's solution to this problem. |
|
I tried testing this, but the G2 components page makes it very hard. I selected Menu and then found the iframe in whhich the content is loaded. When I tried to select the button inside the iframe, nothing happened. The "Code is Poetry" button. It did not appear to toggle aria-expanded either. Maybe I'm looking in the wrong place? |
|
@alexstine As long as you're inside of Storybook you're looking at the right place. We're going to close this PR in favor of #30097 actually so if you have time, testing that PR instead would be better... but it's still in Storybook 😞 |
Description
Adds a new
Menucomponent. We do not write an adapter forMenuItembecause the newMenuItemis only meant to be used inside of thisMenucomponent which did not previously exist. Rather than using an adapter to use the newMenuItem, consumers should update to just use theMenucomponent directly with its correspondingHeaderandItemcomponents.Depends on #29592
How has this been tested?
Run storybook (
npm run storybook:dev), go to the experimental componentsMenuand test that the menu item is selectable and appears correctly. Also test with keyboard navigation to try to focus the menu item and select it using the keyboard (SpaceorEnter)Screenshots
Types of changes
New feature
Checklist: