search

mui / material-ui

Material UI: Ready-to-use foundational React components, free forever. It includes Material UI, which implements Google's Material Design.
https://mui.com/material-ui/
MIT License
91.86k stars 31.57k forks source link

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

Open sai6855 opened 2 weeks ago

sai6855 commented 2 weeks ago

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

mui-bot commented 2 weeks ago

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 :no_entry_sign: dangerJS against 5e28613caccfcebeca093499130ebe08f19a5c75

DiegoAndai commented 2 weeks ago

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 commented 2 weeks ago

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 commented 1 week ago

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 commented 1 week ago

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 https://github.com/mui/material-ui/issues/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 commented 1 week ago

@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 commented 1 week ago

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:

Does that make sense?

aarongarciah commented 1 week ago

@DiegoAndai yes, I think that makes sense

sai6855 commented 1 week ago

@DiegoAndai i went with your suggestion. commit for change -> https://github.com/mui/material-ui/pull/42082/commits/5e28613caccfcebeca093499130ebe08f19a5c75

Summary of changes :

  1. Removed following classes
  1. Added following classes to FilledInput root

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

sai6855 commented 4 days ago

Ping @DiegoAndai