Skip to content

[TextField] Migrate FilledInput to emotion#24617

Closed
duganbrett wants to merge 5 commits into
mui:nextfrom
duganbrett:migrate-filled-input
Closed

[TextField] Migrate FilledInput to emotion#24617
duganbrett wants to merge 5 commits into
mui:nextfrom
duganbrett:migrate-filled-input

Conversation

@duganbrett

Copy link
Copy Markdown
Contributor

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

@mui-pr-bot

mui-pr-bot commented Jan 25, 2021

Copy link
Copy Markdown

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

Details of bundle changes

Generated by 🚫 dangerJS against 7003e0c

@duganbrett

Copy link
Copy Markdown
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 scope: text field Changes related to the text field. label Jan 25, 2021
@mnajdova

Copy link
Copy Markdown
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

oliviertassinari commented Jan 26, 2021

Copy link
Copy Markdown
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?

@mnajdova

Copy link
Copy Markdown
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
Copy Markdown
Member

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

Comment thread packages/material-ui/src/FilledInput/FilledInput.js Outdated
Comment thread packages/material-ui/src/FilledInput/FilledInput.js Outdated
@duganbrett

Copy link
Copy Markdown
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
Copy Markdown
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

scope: text field Changes related to the text field.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants