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

Warn when value and defaultValue props are both specified on input or textarea. #5823

Merged
merged 1 commit into from
Jan 11, 2016
Merged

Conversation

michaelgmcd
Copy link
Contributor

Fixes #5819.

Note that I've been using React for a while but this is my first time contributing.

@michaelgmcd michaelgmcd changed the title Warn when value and defaultValue prop is specified on input or textarea. Warn when value and defaultValue props are both specified on input or textarea. Jan 11, 2016
if (props.value !== undefined && props.defaultValue !== undefined && !didWarnValDefaultVal) {
warning(
false,
'Both `value` and `defaultValue` props on `input` should not be specified. ' +
Copy link
Contributor

Choose a reason for hiding this comment

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

The first sentence in this error message makes it sound like neither of the props should be specified. It says "both... props... should not be specified". Alternative wording:

Input elements must be either controlled or uncontrolled (specify either the value prop, or the defaultValue prop, but not both). Decide between using a controlled or uncontrolled input and remove one of these props. More info: https://fb.me/react-controlled-components

@jimfb
Copy link
Contributor

jimfb commented Jan 11, 2016

@mgmcdermott This looks really great! Can you update the wording of the error messages, as per above, and then I think this is good-to-go.

@michaelgmcd
Copy link
Contributor Author

See the new PR. Glad I could help @jimfb. Nice to get my feet wet contributing as well.

@jimfb
Copy link
Contributor

jimfb commented Jan 11, 2016

Looks good.

Can you squash these two commits into a single commit. You can do this using git rebase -i and then squashing, then do a git push -f to update the branch. We generally like single-commit PRs, because it makes things easier to revert if/when necessary.

@jimfb
Copy link
Contributor

jimfb commented Jan 11, 2016

Oh, sorry, and one more thing I just noticed: defaultChecked is also a react prop, we should do the same check for that - can't specify both checked and defaultChecked.

@michaelgmcd
Copy link
Contributor Author

I'll update to include checked/defaultChecked.

@facebook-github-bot
Copy link

@mgmcdermott updated the pull request.

@michaelgmcd
Copy link
Contributor Author

Would this also apply to a select element using the value and defaultValue props?

@michaelgmcd
Copy link
Contributor Author

Changed my comment, I meant a select element. From the source: Implements a select native component that allows optionally setting the props value and defaultValue.

@jimfb
Copy link
Contributor

jimfb commented Jan 11, 2016

Yes, this should also apply to select element :).

…d props are specified on input, textarea, or select elements
@michaelgmcd
Copy link
Contributor Author

See the latest PR, I think I've covered everything.

@facebook-github-bot
Copy link

@mgmcdermott updated the pull request.

@jimfb
Copy link
Contributor

jimfb commented Jan 11, 2016

This looks great, thanks @mgmcdermott!

jimfb added a commit that referenced this pull request Jan 11, 2016
Warn when value and defaultValue props are both specified on input or textarea.
@jimfb jimfb merged commit 171305f into facebook:master Jan 11, 2016
@jimfb
Copy link
Contributor

jimfb commented Jan 11, 2016

@mgmcdermott If you're looking for another bug related to controlled inputs, you might take a stab at #5821, which would be a great next bug.

@michaelgmcd
Copy link
Contributor Author

Will do! Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants