-
-
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
[Autocomplete] Update unstyled demo to not import Material UI #34060
[Autocomplete] Update unstyled demo to not import Material UI #34060
Conversation
It looks like the CODEOWNERS logic fails on this one 😁. I had to manually check who owns this component: Marija and Michal, and since it's also about base, I picked the latter. |
overflow: 'auto', | ||
maxHeight: 200, | ||
border: '1px solid rgba(0,0,0,.25)', | ||
[`& li.${autocompleteClasses.focused}`]: { | ||
'& li.Mui-focused': { |
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.
It's hardcoded anyway:
option.classList.add('Mui-focused'); |
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.
useAutocomplete needs an overhaul to match the other hooks in Base. It uses different patterns now (like setting a class name, that doesn't happen anywhere else in other hooks).
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.
(like setting a class name, that doesn't happen anywhere else in other hooks).
@michaldudak What would be the alternative in this case? For context: #26181
needs an overhaul to match the other hooks in Base.
I think that it would be great for you to review mui/mui-x#5504. I wanted to raise how the unstyled hook API of the date picker feels a bit different from our useAutocomplete (the useAutocomplete API is a lot inspired by https://www.downshift-js.com/use-combobox).
It uses different patterns now
I think that this would be a great opportunity to review the differences, so we can only keep the best changes.
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.
@michaldudak Bump. I'm curious about two things:
- How is the pattern hook different now, e.g. developers seem to enjoy the current approach: https://twitter.com/shyam_lohar_/status/1571000213388791808 (which also makes the case for a headless first approach, anytime we create a new component, in Joy UI, Material UI, MUI X, we should likely start from the headless DX).
- How to align with MUI X on it, e.g. with @flaviendelangle [AutoComplete] dataSourceConfig not working #5504
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.
It looks like the CODEOWNERS logic fails on this one 😁. I had to manually check who owns this component: Marija and Michal, and since it's also about base, I picked the latter.
The CODEOWNERS file is missing the docs directory. That's why no one showed up as an owner of the changed files.
overflow: 'auto', | ||
maxHeight: 200, | ||
border: '1px solid rgba(0,0,0,.25)', | ||
[`& li.${autocompleteClasses.focused}`]: { | ||
'& li.Mui-focused': { |
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.
useAutocomplete needs an overhaul to match the other hooks in Base. It uses different patterns now (like setting a class name, that doesn't happen anywhere else in other hooks).
Make the demo truly unstyled. Any use of
@mui/material
is a red flag IMHO. I was inspired to work on this (quick-win) while I was looking into https://deploy-preview-5504--material-ui-x.netlify.app/x/react-date-pickers/date-field/#headless-usageBefore
https://mui.com/material-ui/react-autocomplete/#useautocomplete
After
https://deploy-preview-34060--material-ui.netlify.app/material-ui/react-autocomplete/#useautocomplete