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

[base-ui][Menu] Fix navigation of items when 1st item is disabled #39828

Merged
merged 1 commit into from
Nov 28, 2023

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented Nov 10, 2023

@sai6855 sai6855 marked this pull request as draft November 10, 2023 12:34
@sai6855 sai6855 added bug 🐛 Something doesn't work component: menu This is the name of the generic UI component, not the React module! package: base-ui Specific to @mui/base labels Nov 10, 2023
@sai6855 sai6855 changed the title [base-ui][Menu] Fix navigation of items when ist item is disabled [base-ui][Menu] Fix navigation of items when 1st item is disabled Nov 10, 2023
@mui-bot
Copy link

mui-bot commented Nov 10, 2023

Netlify deploy preview

https://deploy-preview-39828--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against fdf304c

@@ -122,9 +122,9 @@ export function useMenu(parameters: UseMenuParameters = {}): UseMenuReturnValue
}, [listboxId, registerPopup]);

React.useEffect(() => {
if (open && highlightedValue === subitemKeys[0] && !isInitiallyOpen.current) {
if (open && highlightedValue && !isInitiallyOpen.current) {
Copy link
Contributor Author

@sai6855 sai6855 Nov 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH i don't have context on why subitemKeys[0] is explicilty used here. instead of focusing subitemKeys[0], focusing highlightedValue seems to fix the issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was suspicious seeing it as well, and noticed the same issue while working on adding support for selectable menu items. The changed logic looks good to me 👍 cc @michaldudak for the final review.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to admit I don't remember what was my reasoning here. It may be I was trying some things out and forgot to remove it. Thanks for taking care of it, @sai6855!

@sai6855 sai6855 marked this pull request as ready for review November 10, 2023 13:08
@DiegoAndai DiegoAndai merged commit 7d52350 into mui:master Nov 28, 2023
22 of 23 checks passed
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Nov 30, 2023
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Dec 1, 2023
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Dec 1, 2023
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Dec 6, 2023
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Dec 6, 2023
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: menu This is the name of the generic UI component, not the React module! package: base-ui Specific to @mui/base
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[base-ui][Menu] navigation of items when 1st item is disabled
5 participants