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.

Sign up for GitHub

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

Jump to bottom

[Autocomplete] can't change option highlight color #23297

Closed
wants to merge 9 commits into from
Closed

[Autocomplete] can't change option highlight color #23297

wants to merge 9 commits into from

Conversation

sujinleeme
Copy link
Contributor

@sujinleeme sujinleeme commented Oct 28, 2020 •
edited
Loading

Fixed: #19692

@mui-pr-bot
Copy link

mui-pr-bot commented Oct 28, 2020 •
edited
Loading

Details of bundle changes

Generated by 🚫 dangerJS against a713a50

sujinleeme
sujinleeme commented Oct 28, 2020
View reviewed changes
packages/material-ui/src/Autocomplete/Autocomplete.js Outdated
Comment on lines 210 to 223
},
'&[data-focus="true"]': {

'&$focused': {
backgroundColor: theme.palette.action.hover,
},
'&:active': {
'&$selected': {
backgroundColor: theme.palette.action.selected,
'&$focused': {
backgroundColor: alpha(
theme.palette.action.selected,
theme.palette.action.selectedOpacity + theme.palette.action.hoverOpacity,
),
},
},
'&[aria-disabled="true"]': {
'&$disabled': {
opacity: theme.palette.action.disabledOpacity,
pointerEvents: 'none',
},
Copy link
Contributor Author

@sujinleeme sujinleeme Oct 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure hover/select styling convention atm. This follows ListItem component's styling.

https://github.com/mui-org/material-ui/blob/d9d93088bffc86b2dd06a2436c170e765b92462e/packages/material-ui/src/ListItem/ListItem.js#L26-L40

oliviertassinari
oliviertassinari requested changes Oct 28, 2020
View reviewed changes
packages/material-ui/src/useAutocomplete/useAutocomplete.js Outdated
@@ -129,6 +129,7 @@ export default function useAutocomplete(props) {
const [focusedTag, setFocusedTag] = React.useState(-1);
const defaultHighlighted = autoHighlight ? 0 : -1;
const highlightedIndexRef = React.useRef(defaultHighlighted);
const [focusedIndex, setFocusedIndex] = React.useState(defaultHighlighted);
Copy link
Member

@oliviertassinari oliviertassinari Oct 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It's too slow to rerender for this state. Try https://deploy-preview-23297--material-ui.netlify.app/components/autocomplete/#virtualization

Suggested change
const [focusedIndex, setFocusedIndex] = React.useState(defaultHighlighted);

Copy link
Contributor Author

@sujinleeme sujinleeme Oct 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Can we improve performance for large options using React.useMemo()?

https://github.com/mui-org/material-ui/blob/81aaa6f7ce18ad0b289ee8f0d73c0a20636e9cd9/packages/material-ui/src/Autocomplete/Autocomplete.js#L407-L423

Copy link
Member

Choose a reason for hiding this comment

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

Maybe, virtualization is one case, we also need to consider when people use the render option API with "heavy" components, like the MenuItem.

packages/material-ui/src/Autocomplete/Autocomplete.js Outdated
backgroundColor: theme.palette.action.selected,
'&$focused': {
backgroundColor: alpha(
Copy link
Member

@oliviertassinari oliviertassinari Oct 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Nice! However, this change is related to #18136. I think that we should have a different pull request for it. It would deliver value, on its own :).

Copy link
Contributor Author

@sujinleeme sujinleeme Oct 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@oliviertassinari I changed the above classes names to unique names optionFocused, optionSelected, optionDisabled because official doc says focused is Pseudo-class applied to the root element if focused. Thanks for your review. 🙇‍♀️

Copy link
Contributor Author

@sujinleeme sujinleeme Oct 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

In terms of #18136, I will send a different PR. 🍀

@github-actions github-actions bot mentioned this pull request Oct 29, 2020
Closed
oliviertassinari
oliviertassinari reviewed Oct 29, 2020
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Interesting, I think that we could indeed add the .Mui-disabled, .Mui-focused class names even if we keep using the aria attributes for styling just for consistency. Regarding the original issue, I think that it was about replacing the data attribute with .Mui-focused for consistency with the other component. I think that we should focus on this change, solve one problem at the time.

@oliviertassinari
Copy link
Member

@sujinleeme Thanks for keeping iterating on it. However, I don't think that we can move in this direction. How about we try to get a step back with this plan:

  1. We close the pull request, resume it into different efforts.
  2. We start by solving the CSS issue. I think that you were completely right about using the PaginationItem's CSS to update the styles. We need to align the Autocomplete with the Select components. This is related to [Select] Consistency with Autocomplete #18136 and more importantly: Implement theme level states design tokens #10870. It would allow to ✅ the Autocomplete checkbox.
  3. We solve [Autocomplete] can't change option highlight color #19692 by replacing data-focus with the .Mui-focused class name. This is purely for consistency. We could also apply .Mui-selected and .Mui-disabled as an alternative to using the a11y attributes. Why not, happy to discuss it.
  4. We can consider moving highlightedIndex from a ref to a state but we would need to justify it with an upside. [Autocomplete] New API to control the highlighted option #20852 is related to doesn't seem to move in this direction.

What do you think?

@sujinleeme
Copy link
Contributor Author

Thanks, @oliviertassinari.
That's good idea. I will close this PR and send another one following your suggestion. :)

@sujinleeme sujinleeme closed this Oct 30, 2020
@sujinleeme sujinleeme deleted the issue/19692 branch October 30, 2020 10:44
@sujinleeme sujinleeme mentioned this pull request Oct 30, 2020
Merged
@zannager zannager added the component: autocomplete This is the name of the generic UI component, not the React module! label Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviertassinari oliviertassinari oliviertassinari requested changes

Assignees
No one assigned
Labels
component: autocomplete This is the name of the generic UI component, not the React module!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Autocomplete] can't change option highlight color
4 participants
@sujinleeme @mui-pr-bot @oliviertassinari @zannager