-
-
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
[material-ui][FilledInput] Remove unapplied classes from filledInputClasses interface and add missing classes to root #42082
Conversation
Netlify deploy previewhttps://deploy-preview-42082--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
This is a difficult one. With the current architecture, it seems more useful for users that each input hosts their own classes. This might also change when we update the styles and theme structure, so we might want to avoid breaking changes until then. What do you think @aarongarciah |
I think most of the above classes selector behavior can be achieved by combining the FilledInput root class and the So we better go direction of removing them |
The problem is also present in @DiegoAndai I'm inclined to think that every input should host their own classes so consumers can target them individually. If they want to target elements or states common to all inputs, they can target the @sai6855 even when |
@aarongarciah I'm not sure if you are aware of this issue #41282 but TL DR; of the issue is, we are deprecating composed classes which can be achieved through css selctors. For example docs link for same: https://deploy-preview-41740--material-ui.netlify.app/material-ui/migration/migrating-from-deprecated-apis/#composed-css-classes-8 Since we already went with above approach, i was recomending |
@sai6855 TIL! Thanks for bringing it to my attention. Then removing composed classes that are unused makes sense. Forget my previous comment except:
|
I think option 3: From that list, remove the composed classes, and add the ones that are not composed. So for example:
Does that make sense? |
@DiegoAndai yes, I think that makes sense |
@DiegoAndai i went with your suggestion. commit for change -> 5e28613 Summary of changes :
Once this PR is merged, i'll replicate same changes to other Inputs |
2945477
to
81e10af
Compare
81e10af
to
5e28613
Compare
Ping @DiegoAndai |
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.
Thanks @sai6855!
…lasses interface and add missing classes to root (mui#42082)
Documentation says following classes are applied when necessary conditions are met but actually none of the classes are applied. I've written test case to validate same here
we can do either of the following
@DiegoAndai which approach you recommend