-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
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] Migrate NativeSelect to emotion #24698
Conversation
The styles from the |
@material-ui/core: parsed: +0.37% , gzip: +0.20% |
…grate-native-select
Would be great to have this in the next release 😁 |
@mnajdova Interested in taking this one on? It's a challenging one. I doubt the current solution is flying, it might need a maintainer to solve it. |
Sure, will do |
Ok, this is a bit tricky :)) From what I can see we should do the following:
I will try to implement these changes tomorrow on top of this PR. Alternative would be to use the |
@mnajdova The part that seemed tricky to me was that, since Select and NativeSelect aim to share most of the logic, we would need to migrate NativeSelect first, then export the component, for Select. For the solution, instead of a |
I think it would be best if we convert the |
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.
- 22
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.
I did some polishing and based the #25653 on top of this one. I don't think there is anything else we are missing. Actually, it was pretty straightforward to do it, we just needed to export the styles from the NativeSelectInput
(I know you had those changes initially, but I moved them to the other PR so that it would be more obvious why those are needed.)
cc @oliviertassinari in case you are interested
Thanks for cleaning this one up and welcome back! |
I am merging this one, we can revisit if anything else is necessary on #25653 |
@@ -62,7 +62,7 @@ export const styles = (theme) => ({ | |||
}, | |||
/* Styles applied to the select component `selectMenu` class. */ | |||
selectMenu: { | |||
height: 'auto', // Resets for multiple select with chips | |||
height: 'auto', // Resets for multipile select with chips |
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.
height: 'auto', // Resets for multipile select with chips | |
height: 'auto', // Resets for multiple select with chips |
}, | ||
}), | ||
...(styleProps.selectMenu && { | ||
height: 'auto', // Resets for multpile select with chips |
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.
height: 'auto', // Resets for multpile select with chips | |
height: 'auto', // Resets for multiple select with chips |
@@ -1,2 +1,4 @@ | |||
export { default } from './NativeSelect'; | |||
export * from './NativeSelect'; | |||
export { default as nativeSelectClasses } from './nativeSelectClasses'; |
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.
Convention with the other files
export { default as nativeSelectClasses } from './nativeSelectClasses'; | |
export { default as nativeSelectClasses } from './nativeSelectClasses'; |
@@ -1 +1,3 @@ | |||
export { default } from './NativeSelect'; | |||
export { default as nativeSelectClasses } from './nativeSelectClasses'; |
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.
Convention with the other files
export { default as nativeSelectClasses } from './nativeSelectClasses'; | |
export { default as nativeSelectClasses } from './nativeSelectClasses'; |
@oliviertassinari requested changes addressed in 7b09f14 |
This PR migrates the NativeSelect and NativeSelect Input components to the new emotion format as a part of #24405.