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

[Select][base-ui] Removes highlighted class on item deselection #38790

Closed
wants to merge 16 commits into from

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented Sep 3, 2023

closes: #38789

This PR fixes 2 issues described below.

steps to reproduce

Issue 1

before:

  1. open https://mui.com/base-ui/react-select/#multi-select
  2. click on select
  3. deselect 20 and move mouse away.
  4. option is still highlighted

after:

  1. open https://deploy-preview-38790--material-ui.netlify.app/base-ui/react-select/#multi-select
  2. click on select
  3. deselect 20 and move mouse away.
  4. option is not highlighted

Issue 2

before:

  1. open https://mui.com/base-ui/react-select/#multi-select
  2. click on select
  3. hover mouse on 30 option
  4. click space button
  5. observe ten option is deselected where infact 30 should be selected.
    (benchmark for this behaviour: https://ariakit.org/examples/select-multiple )

after:

  1. open https://deploy-preview-38790--material-ui.netlify.app/base-ui/react-select/#multi-select
  2. click on select
  3. hover mouse on 30 option
  4. click space button
  5. 30 is selected.

@sai6855 sai6855 added bug 🐛 Something doesn't work component: select This is the name of the generic UI component, not the React module! package: base-ui Specific to @mui/base labels Sep 3, 2023
@mui-bot
Copy link

mui-bot commented Sep 3, 2023

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against ef4a51d

@@ -217,10 +217,11 @@ function handleItemSelection<ItemValue, State extends ListState<ItemValue>>(
// if the item is already selected, remove it from the selection, otherwise add it
const newSelectedValues = toggleSelection(item, selectedValues, selectionMode, itemComparer);

const highlightedValue = selectedValues.includes(items[itemIndex]) ? null : item;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

since itemIndex is calculated by taking itemComparer into consideration, i reused itemIndex here.

@sai6855 sai6855 requested a review from michaldudak September 3, 2023 15:47
@sai6855 sai6855 changed the title [Select][base-ui] Fix applying highlighted class [Select][base-ui] Removes highlighted class on item deselection Sep 3, 2023
@michaldudak
Copy link
Member

IMO the UX is worse now. When I use the keyboard to deselect an option, I don't want the highlight to be reset, but to stay on the option I just deselected so I can move to other options.

@sai6855
Copy link
Contributor Author

sai6855 commented Sep 4, 2023

IMO the UX is worse now. When I use the keyboard to deselect an option, I don't want the highlight to be reset, but to stay on the option I just deselected so I can move to other options.

Got it. What about highlighting reset only when user clicks on option? If user deselects through keyboard option will stay highlighted

@sai6855
Copy link
Contributor Author

sai6855 commented Sep 4, 2023

@michaldudak what do you think about current UX. I've fixed UX issue you described, additionally other issue also got fixed automatically (you can find this in PR description)

@michaldudak
Copy link
Member

michaldudak commented Sep 8, 2023

What about highlighting reset only when user clicks on option? If user deselects through keyboard option will stay highlighted

I'm not convinced it's the right approach. Since I interacted with a given option, it makes sense to be focused (= highlighted).

As for issue 2, it's a controversial topic. IMO keyboard and mouse highlights should be separate (= the current behavior). We could make it more apparent by changing the option hover style so it's different from the highlighted state.

@sai6855 sai6855 closed this Sep 9, 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: select 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.

[Select][base-ui] Incorrect highlighted class applied on Select with multiple
3 participants