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

[TextField] Migrate FilledInput to emotion #24617

Closed
wants to merge 5 commits into from
Closed

[TextField] Migrate FilledInput to emotion #24617

wants to merge 5 commits into from

Conversation

duganbrett
Copy link
Contributor

This PR migrates the FilledInput component to the new emotion format as a part of #24405.

@mui-pr-bot
Copy link

mui-pr-bot commented Jan 25, 2021

@material-ui/core: parsed: +0.14% , gzip: +0.17%

Details of bundle changes

Generated by 🚫 dangerJS against 7003e0c

@duganbrett
Copy link
Contributor Author

Very open to guidance on this one. Some of the way classes need to be applied on the input variants is not that obvious to me.

@oliviertassinari oliviertassinari added the component: text field This is the name of the generic UI component, not the React module! label Jan 25, 2021
@mnajdova
Copy link
Member

Very open to guidance on this one. Some of the way classes need to be applied on the input variants is not that obvious to me.

This was a bit trickier than what I anticipated :) I tried to fix all issues with a73218b (let's see if everything is resolved) We need to be careful when styling existing component regarding which things are coming from state (for example hiddenLabel, which things come from context etc). For these we must use the classes selector coming from that component, for the rest we can safely use the styleProps.

Regarding the overrides & classes, I belive we should add only the things which are new to this component, in this case the new names for the slots (MuiFilledInput-root & MuiFilledInput-input + the classes for the new underlined class key) Cc @oliviertassinari

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 26, 2021

This one does look tricky. I'm not sure about the increase of specificity. Should we implement the components prop in the InputBase and wrap the nested InputBaseInput with styled instead?

@mnajdova
Copy link
Member

This one does look tricky. I'm not sure about the increase of specificity. Should we implement the components prop in the InputBase and wrap the nested InputBaseInput with styled instead?

That is the other option, yeah. Let me prepare a different PR for it and see which one will work better 👍

@mnajdova
Copy link
Member

@oliviertassinari prepared #24634 I think it's better if we use components & componentsProps, we are able to avoid any increasing of specificity

@duganbrett
Copy link
Contributor Author

@mnajdova @oliviertassinari Thank you, both! Learning a lot. I hope this helps others with the remaining TextField variants. Feel free to close.

@mnajdova
Copy link
Member

@mnajdova @oliviertassinari Thank you, both! Learning a lot. I hope this helps others with the remaining TextField variants. Feel free to close.

Thanks for exploring 🙏 , this was a tricky one. Hopefully now #24634 can serve as a template for the other input components. Closing this one.

@mnajdova mnajdova closed this Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: text field This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants