-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
refactor(menu): use react.context and simplify logic #13146
Conversation
✅ Deploy Preview for carbon-components-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
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.
Mostly LGTM, some cleanup suggestions.
Two more things:
- I'm experiencing some errors when opening a menu for the second time and trying to use keyboard navigation (see console. I am arrowing down in this example):
Screen.Recording.2023-02-15.at.12.21.19.PM.mov
- Should bullet points As a user I want to have widgets for graphing data (circles and bar graphs) #2.5 & Hiding the menu does not hide the menu in NVDA #4 be addressed in this PR or is that outside of the scope?
ContextMenu: Bring tostable
☂️ #11770 (comment)
|
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.
LGTM!
I'm still noticing some unexpected keyboard behaviors:
Screen.Recording.2023-02-16.at.10.23.28.AM.mov |
@francinelucca Thanks for catching that! It should be fixed now. Let me know if you find anything else! |
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.
Behavior LGTM now 🔥. Just one more thing I found on the story 🙏🏻
Hey there! v11.23.1 was just released that references this issue/PR. |
Contributes to #11770
Menu
to useReact.Context
in order to avoid direct DOM manipulation like beforeChangelog
Changed
MenuGroup
toMenuItemGroup
MenuDivider
toMenuItemDivider
MenuRadioGroup
toMenuItemRadioGroup
MenuSelectableItem
toMenuItemSelectable
MenuItem.js
file to avoid cyclic dependencyMenu
exportforwardRef
to all componentsMenu
Menu
to useReact.Context
instead of direct DOM manipulationsxs
sizeMenuItemRadioGroup
props.itemToString
for custom functions to convert items to stringsprops.selectedItem
MenuItemSelectable
props.selected
Removed
Testing / Reviewing