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

Allow setting defaultValue as a property for #4618 #5666

Closed
wants to merge 1 commit into from

Conversation

hayesgm
Copy link

@hayesgm hayesgm commented Dec 15, 2015

Fixes issue #4618, which allows updates to defaultValue on uncontrolled <input>s. Browsers will reflect changes to defaultValue unless the user has changed the input value. This patch also allows updates to defaultValue on a controlled element with value == null.

@spicyj suggested using setAttribute('value'), but instead this patch sets the defaultValue property as a property. The goal is to keep behavior as consistent with current day as possible.

If we like this approach, we should consider bringing it to <textarea> and <select>, as well.

Here's a simple test of the patch on JSBin: https://jsbin.com/yagasi/edit?js,output

checked: checked != null ? checked : inst._wrapperState.initialChecked,
onChange: inst._wrapperState.onChange,
});

if (value !== undefined) {
// for contolled inputs, use defaultValue as an initial value
Copy link
Contributor

Choose a reason for hiding this comment

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

cont_r_olled

@hayesgm hayesgm force-pushed the default-value-updates branch from bfab5fe to a428cfa Compare December 16, 2015 21:26
@hayesgm
Copy link
Author

hayesgm commented Dec 16, 2015

@yaycmyk Thanks, fixed.

@facebook-github-bot
Copy link

@hayesgm updated the pull request.

@hayesgm
Copy link
Author

hayesgm commented Dec 16, 2015

I built an alternate approach and opened a pull request via #5680. Both are functional and it's a matter of style to pick between these two.

@sophiebits
Copy link
Collaborator

This requires fewer changes but #5680 is closer to where I'd like to end up since we're moving away from properties and towards attributes in most cases (e.g., see #1510). Plus with #5680 it looks like we may be able to kill "hasSideEffects" since that logic will be inlined into the form components that need it and no other properties do.

So let's go with that one.

@sophiebits sophiebits closed this Dec 16, 2015
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.

4 participants