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] Remove excessive catching of hiddenLabel prop #22444

Merged

Conversation

croraf
Copy link
Contributor

@croraf croraf commented Sep 1, 2020

The prop is only forwarded to the FormControl component, so no need to catch it.

This should also be done on the next branch. Haven't found a section in the docs which explains to which branches (that is master or next) the new commits should land. Perhaps non-critical and new features should land only on next.

@mui-pr-bot
Copy link

mui-pr-bot commented Sep 1, 2020

Details of bundle changes

Generated by 🚫 dangerJS against fcaef64

@oliviertassinari
Copy link
Member

@croraf Smart

@oliviertassinari oliviertassinari added component: text field This is the name of the generic UI component, not the React module! performance labels Sep 1, 2020
@oliviertassinari oliviertassinari changed the title [TextField] remove excessive catching of hiddenLabel prop [TextField] Remove excessive catching of hiddenLabel prop Sep 1, 2020
@oliviertassinari oliviertassinari changed the base branch from master to next September 1, 2020 22:07
@oliviertassinari oliviertassinari force-pushed the remove_excessive_catching_of_hiddenLabel branch from 853e977 to fcaef64 Compare September 1, 2020 22:07
@oliviertassinari oliviertassinari merged commit 669a0b3 into mui:next Sep 1, 2020
@oliviertassinari
Copy link
Member

@croraf Thanks for reporting it, I have moved it to the next branch

@croraf
Copy link
Contributor Author

croraf commented Sep 1, 2020

@oliviertassinari I saw some other properties with the same behavior, so I was wondering if this "unnecessary" exposing is perhaps done with intention (for example to improve DX intellisense prop values suggestions)?

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 2, 2020

Do you have an example?

@croraf
Copy link
Contributor Author

croraf commented Sep 2, 2020

Do you have an example?

If you mean the example of unused prop (and not the example Intellisense usage). I'm checking now again. And none is really "completely unused". But there are some of them that are "effectively unused".

Let me explain. In TextField component the props disabled, error, color are passed through to the FormControl root element. They are unused otherwise except that their default value is set during their decomposition from props (https://github.com/mui-org/material-ui/blob/next/packages/material-ui/src/TextField/TextField.js#L58-L92). disabled and error are set to false and color is set to primary. The thing is, these three props will be set to that same default values in the underlying FormControl component (https://github.com/mui-org/material-ui/blob/f90c864e4648cb87d785576cfb6fbb36caf2ec24/packages/material-ui/src/FormControl/FormControl.js#L63-L79).

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 2, 2020

@croraf I think that we include them to retain the generation of the default prop in the API docs and to make it obvious what the default values are. I'm not sure it's worth refactoring.

@croraf
Copy link
Contributor Author

croraf commented Sep 2, 2020

But the default values are defined on the underlying component and the API says "Any other props supplied will be provided to the root element (FormControl)".

Following the other logic we can return the hiddenLabel in the TextField and add its default to false as this is the default on the underlying FormControl. This will be consistent. But then all other underlying root props should also be exposed in the parent's (ancestors') props.

@oliviertassinari
Copy link
Member

@croraf Yeah, I had this thought too, I believe that Sebastian had made hiddenLabel @ignored because it's not the kind if prop we want to encourage the usage of (for a11y). I also believe that better exposing the documentation of nested component could get us a long way: #18288.

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! performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants