-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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
[Input] should accommodate number and string values #7791
Conversation
This works and doesn't break anything, but I need to do a bit more with |
… numbers This is based on the previous implementation, still not sure that 0 !isDirty
Ok, this is good to review. In order to track the details, I've tried to be as explicit as possible with documentation and tests. The issues were two fold:
|
BTW - I have tested it locally on docs and my own number field. |
src/Input/Input.js
Outdated
isControlled() { | ||
return typeof this.props.value === 'string'; | ||
const { onChange, value } = this.props; |
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.
onChange shouldn't be taken into account. React isn't using it for native components.
src/Input/Input.js
Outdated
} | ||
|
||
/** | ||
* Determine if field is dirty (a.k.a. filled). |
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.
The master branch was looking for invalid values instead of valid ones. Have you considered this option https://github.com/callemall/material-ui/blob/master/src/TextField/TextField.js#L110 ? Maybe we could make the implementation shorter/simpler.
Refactored with logic from |
The previous
isControlled()
was too naive and not in-line with the definition of a controlled component as defined by React. This previous implementation was the root cause of failures to trigger the dirty state on the field.