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

[material-ui][FilledInput] Remove unapplied classes from filledInputClasses interface and add missing classes to root #42082

Merged
merged 3 commits into from
May 30, 2024

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented May 1, 2024

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

  1. .MuiFilledInput-sizeSmall
  2. .MuiFilledInput-multiline
  3. .MuiFilledInput-adornedEnd
  4. .MuiFilledInput-adornedStart
  5. .MuiFilledInput-inputSizeSmall
  6. .MuiFilledInput-inputMultiline
  7. .MuiFilledInput-inputAdornedStart
  8. .MuiFilledInput-inputAdornedEnd
  9. .MuiFilledInput-inputTypeSearch

we can do either of the following

  1. to remove above classes from filledInputClasses interface and from docs. that's what this PR does
  2. to add above classes to applicable elements and don't remove any classes from filledInputClasses interface

@DiegoAndai which approach you recommend

@sai6855 sai6855 added bug 🐛 Something doesn't work package: material-ui Specific to @mui/material component: input This is the name of the generic UI component, not the React module! labels May 1, 2024
@mui-bot
Copy link

mui-bot commented May 1, 2024

Netlify deploy preview

https://deploy-preview-42082--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 5e28613

@sai6855 sai6855 marked this pull request as draft May 1, 2024 12:34
@sai6855 sai6855 marked this pull request as ready for review May 1, 2024 12:53
@sai6855 sai6855 requested a review from DiegoAndai May 1, 2024 12:53
@DiegoAndai
Copy link
Member

This is a difficult one. InputBase also has these classes, should that component host them, or should each of the inputs (Standard, Outlined, Filled) host them 🤔

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

@sai6855
Copy link
Contributor Author

sai6855 commented May 3, 2024

I think most of the above classes selector behavior can be achieved by combining the FilledInput root class and the has selector. Even if we add them now, they can be deprecated and removed in the future. (Once has is supported in enough browsers)

So we better go direction of removing them

@aarongarciah
Copy link
Member

aarongarciah commented May 7, 2024

The problem is also present in Input, FilledInput, and OutlineInput. So we should apply any changes to those components, too.

@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 InputBase classes.

@sai6855 even when :has is widely supported, I think is very valuable for consumers to have these static classes they can target as part of the components' public APIs. By removing classes, consumers would need to know about components' implementation details that classes help abstract.

@sai6855
Copy link
Contributor Author

sai6855 commented May 7, 2024

By removing classes, consumers would need to know about components' implementation details that classes help abstract.

@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 .MuiStepConnector-lineHorizontal is deprecated in favour of .MuiStepConnector-horizontal > .MuiStepConnector-line, so with this change, consumers of compoents are expected to know internal structure of componets.

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 has approach

@aarongarciah
Copy link
Member

@sai6855 TIL! Thanks for bringing it to my attention. Then removing composed classes that are unused makes sense. Forget my previous comment except:

The problem is also present in Input, FilledInput, and OutlineInput.

@DiegoAndai
Copy link
Member

we can do either of the following

  1. to remove above classes from filledInputClasses interface and from docs. that's what this PR does
  2. to add above classes to applicable elements and don't remove any classes from filledInputClasses interface

@DiegoAndai which approach you recommend

I think option 3:

From that list, remove the composed classes, and add the ones that are not composed. So for example:

  • Add .MuiFilledInput-sizeSmall
  • Remove .MuiFilledInput-inputAdornedStart. No need to deprecate it if it never worked

Does that make sense?

@aarongarciah
Copy link
Member

@DiegoAndai yes, I think that makes sense

@sai6855
Copy link
Contributor Author

sai6855 commented May 9, 2024

@DiegoAndai i went with your suggestion. commit for change -> 5e28613

Summary of changes :

  1. Removed following classes
  • .MuiFilledInput-inputSizeSmall
  • .MuiFilledInput-inputMultiline
  • .MuiFilledInput-inputAdornedStart
  • .MuiFilledInput-inputAdornedEnd
  • .MuiFilledInput-inputTypeSearch
  1. Added following classes to FilledInput root
  • .MuiFilledInput-sizeSmall
  • .MuiFilledInput-multiline
  • .MuiFilledInput-adornedEnd
  • .MuiFilledInput-adornedStart
  • .MuiFilledInput-hiddenLabel

Once this PR is merged, i'll replicate same changes to other Inputs

@sai6855 sai6855 changed the title [material-ui][FilledInput] Remove unapplied classes from filledInputClasses interface [material-ui][FilledInput] Remove unapplied classes from filledInputClasses interface and add missing classes to root May 9, 2024
@sai6855
Copy link
Contributor Author

sai6855 commented May 15, 2024

Ping @DiegoAndai

Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @sai6855!

@DiegoAndai DiegoAndai merged commit 090850a into mui:next May 30, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: input This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants