Navigation Menu

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 inputClassName property #9509

Merged
merged 3 commits into from Dec 16, 2017
Merged

[TextField] Remove inputClassName property #9509

merged 3 commits into from Dec 16, 2017

Conversation

kgregory
Copy link
Member

@kgregory kgregory commented Dec 15, 2017

Fixes #9508

For TextField, inputClassName should be applied to the input element whether InputProps includes inputProps or not. The one exception would be if the inner inputProps includes a className prop as well.

Breaking change

The existing InputProps property can be used to set the className on the input element, making inputClassName redundant. Issue #9508 exposed some conflicting behavior between the two properties and it was decided that removing inputClassName would result in a cleaner API.

-  /**
-   * The CSS class name of the `input` element.
-   */
-  inputClassName: PropTypes.string,

The configuration of the wrapped Input component and its input element should be done through InputProps. To specify a className on the input element:

<TextField InputProps={{ inputProps: { className: 'foo' } }} />

inputClassName should be applied to the input element whether InputProps includes inputProps or not. The one exception would be if the inner inputProps includes a className prop as well.
@oliviertassinari
Copy link
Member

Maybe the solution is to remove inputClassName

@kgregory
Copy link
Member Author

@oliviertassinari that would be cleaner. If that's the consensus, I'll update the PR.

@rosskevin
Copy link
Member

rosskevin commented Dec 15, 2017

I debated removing inputClassName before. On TextField, it is flatly exposing a field that is propagated to a great grandchild, and it is ultimately className. I really wonder about these convenience props when we have the uppercase Component props. It's hard to determine where the line is.

But, I do know where I stand, and that is with clarity. I would kill all of these synthetic props and only allow you to pass via the upper case component props itself. I'm sure I could be convinced to expose things for high use scenarios, but otherwise anything advanced should just be through the upper case component props.

@kgregory
Copy link
Member Author

@rosskevin I agree with you. InputProps should be enough for TextField. inputProps should be enough for Input. The subset of input attributes that have been added to both of these components is incomplete and will always cause confusion. Maybe some have merit, like name, but I share your stance on this.

@oliviertassinari
Copy link
Member

@kgregory Sounds like we have a plan. You can make the PR much simpler by removing inputClassName. A breaking change section like here would be perfect for when we edit the release note.

@oliviertassinari oliviertassinari added the component: text field This is the name of the generic UI component, not the React module! label Dec 15, 2017
@oliviertassinari oliviertassinari changed the title [TextField] apply inputClassName and inputProps (via InputProps) [TextField] Remove inputClassName property Dec 15, 2017
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Let's make things simpler :)

@kgregory
Copy link
Member Author

@oliviertassinari all set, thanks again.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

It's perfect 👌

@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Dec 16, 2017
@oliviertassinari oliviertassinari merged commit 18eaea2 into mui:v1-beta Dec 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change bug 🐛 Something doesn't work component: text field This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants