-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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][material-ui][joy-ui][useAutocomplete] Correct keyboard navigation with multiple disabled options #38788
[base-ui][material-ui][joy-ui][useAutocomplete] Correct keyboard navigation with multiple disabled options #38788
Conversation
Netlify deploy previewhttps://deploy-preview-38788--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
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.
The logic looks good to me. Regarding the tests, so far most of the tests were added in Material UI's Autocomplete
component, as a way to test more the end to end behaviour, however this will likely change long term considering all components will use the hook, it would mean that we need to replicate the tests in all components. cc @michaldudak for thoughts on this.
The goal is to test the Base UI unstyled component thoroughly, so we have coverage of both component and a hook. Material UI and Joy UI could have just their extensions tested. |
Hi @mnajdova ! |
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.
@VadimZvf Looks good! Also nice tests 👍 I've left some minor comments.
Your PR fixes #39281. I linked it in the description. |
Thank you for reviewing 🙏 CI task has failed, seems to me like something with the CI tool |
…ttps://github.com/VadimZvf/material-ui into fix-keyboard-navigation-for-disabled-select-options
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.
@VadimZvf Looks good! Thank you for your contribution!
Fixes #39281
The current implementation of the useAutocomplete hook has a little bug.
The looped navigation in the option list doesn't work properly.
When user navigates via keyboard in the select component with disabled options, he needs to press the down arrow twice if disabled option is in the end of the list.
Scenario:
Example before fix:
Screen.Recording.2023-09-03.at.14.26.02.mov
After fix:
Screen.Recording.2023-09-03.at.14.48.08.mov
Issue codesandbox: https://codesandbox.io/s/agitated-frost-qcdgxt?file=/Demo.tsx
Fix codesandbox: https://codesandbox.io/s/autumn-wood-tcc2jl?file=/package.json
Another fix of #39281: https://codesandbox.io/s/https-github-com-mui-material-ui-issues-39281-5g5qsf?file=/Demo.tsx