-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
Make Input component more useful (respects error prop declaratively) #98
Conversation
Make Input component more useful (respects error prop declaratively)
}, | ||
|
||
render: function() { | ||
var classes = this.getClasses('mui-input', { | ||
'mui-floating': this.props.inputStyle === 'floating', | ||
'mui-text': this.props.type === 'text', | ||
'mui-error': this.state.error === true | ||
'mui-error': this.props.error !== undefined && this.props.error !== null |
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.
error != null
matches both undefined
and null
Better to use: componentWillReceiveProps: function (nextProps) {
if (nextProps.error) {
this.setState({
error: nextProps.error
});
}
}, |
I'm not a frequent React user so I might be confused here, but I recall reading that using prop values for the state of the component is bad because it duplicates the "source of truth" (eg. all info is there in the props, so why pull it into the state?). |
Yes, removing the set/removeError methods would most idiomatic. |
I've changed the way the and
Input
component decides whether there is an error or not.Input
doesn't have any state related to errors any more. Instead the error is hold completely inside props.Some examples:
This is more declarative than having to call
setError
andremoveError
and allows components touse the
Input
more concisely.Additionally I also made
type="text"
default for theInput
(although this is something debatable).