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

[ListboxUnstyled] Fix option state highlighted to prevent unnecessary focus #35838

Merged
merged 5 commits into from
Jan 31, 2023

Conversation

SaidMarar
Copy link
Contributor

@SaidMarar SaidMarar commented Jan 15, 2023

close #35780

This fix the bad focus on options when the list is initialized, In this case the findIndex will always return -1 for the index of the option because the array options is not filled yet.

before: https://codesandbox.io/s/k0zipv?file=/demo.js:0-1083
after: https://codesandbox.io/s/fix-listbox-uvx06z?file=/demo.js

Problem

highlighted: highlightedIndex === index, // this will be always true as long as the options array is empty

Fix

Force condition by checking index is not equal to -1 when the options is empty

@mui-bot
Copy link

mui-bot commented Jan 15, 2023

Messages
📖 Netlify deploy preview: https://deploy-preview-35838--material-ui.netlify.app/

Details of bundle changes

Generated by 🚫 dangerJS against 1a5c1ce

@zannager zannager added component: menu This is the name of the generic UI component, not the React module! design: joy This is about Joy Design, please involve a visual or UX designer in the process labels Jan 16, 2023
@zannager zannager requested a review from mnajdova January 16, 2023 08:39
@siriwatknp siriwatknp requested review from michaldudak, mj12albert and hbjORbj and removed request for mnajdova January 17, 2023 03:49
@siriwatknp siriwatknp added the PR: needs test The pull request can't be merged label Jan 17, 2023
@mnajdova mnajdova requested review from siriwatknp and removed request for michaldudak and hbjORbj January 17, 2023 10:14
@mnajdova mnajdova added component: list This is the name of the generic UI component, not the React module! package: base-ui Specific to @mui/base and removed component: menu This is the name of the generic UI component, not the React module! design: joy This is about Joy Design, please involve a visual or UX designer in the process labels Jan 17, 2023
@mnajdova mnajdova requested review from michaldudak and removed request for siriwatknp January 17, 2023 10:16
@SaidMarar
Copy link
Contributor Author

I updated the broken link test of the fix: https://codesandbox.io/s/fix-listbox-uvx06z?file=/demo.js

@michaldudak
Copy link
Member

Thanks for the fix. Would you mind adding a test to verify the behavior?

@SaidMarar
Copy link
Contributor Author

Thanks for the fix. Would you mind adding a test to verify the behavior?

@michaldudak done check it out 😉

@SaidMarar SaidMarar requested review from michaldudak and removed request for mj12albert January 29, 2023 11:37
Copy link
Member

@michaldudak michaldudak left a comment

Choose a reason for hiding this comment

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

This looks good. Thanks for your contribution!

@ZeeshanTamboli ZeeshanTamboli removed the PR: needs test The pull request can't be merged label Jan 30, 2023
@hbjORbj hbjORbj merged commit 94861ea into mui:master Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: list 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.

[Joy][Menu] Scrollable menu jumps to end and back to start on load
8 participants