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.

Sign up for GitHub

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

Jump to bottom

Fixes initial option select #5362

Merged
merged 1 commit into from
May 9, 2016
Merged

Fixes initial option select #5362

merged 1 commit into from
May 9, 2016

Conversation

yiminghe
Copy link
Contributor

@yiminghe yiminghe commented Nov 2, 2015

make sure initial select

<select value="2">
<option>1</option>
<option>2</option>
</select>

renders correctly.

Update is fine, because native option.value will get its content if value is absent

@facebook-github-bot
Copy link

@yiminghe updated the pull request.

@jimfb
Copy link
Contributor

jimfb commented Nov 2, 2015

Looks good to me. Will leave it open for a day or two so everyone can take a look before we merge.

sophiebits
sophiebits reviewed Nov 3, 2015
View reviewed changes
src/renderers/dom/client/wrappers/ReactDOMOption.js
@@ -41,31 +41,38 @@ var ReactDOMOption = {
// or missing (e.g., for <datalist>), we don't change props.selected
var selected = null;
if (selectValue != null) {
var value;
if ('value' in props) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

props.value != null please – we treat null and undefined the same as omitted props for all DOM components.

@sophiebits
Copy link
Collaborator

We should call getNativeProps and mountWrapper in the same order for all components – it is too confusing to call getNativeProps first in some cases and mountWrapper first in other cases.

@yiminghe
Copy link
Contributor Author

yiminghe commented Nov 3, 2015

Yes, I also feel weird for option to have a different order from others, then we have to call flattenChildren twice if value is omitted....

@facebook-github-bot
Copy link

@yiminghe updated the pull request.

@facebook-github-bot
Copy link

@yiminghe updated the pull request.

@jimfb
Copy link
Contributor

jimfb commented Nov 5, 2015

If we warn inside flattenChildren, and flattenChildren gets called twice, it seems like we would warn twice, right? We might want to move the warning logic out of flattenChildren.

Also, let's squash into the two commits into a single commit before merging.

Otherwise, looks good to me. @spicyj?

@sophiebits
Copy link
Collaborator

Yeah. Can we just set a global flag so that we only warn once?

@yiminghe
Copy link
Contributor Author

yiminghe commented Nov 5, 2015

Just as before, only warn in getNativeProps

@jimfb
Copy link
Contributor

jimfb commented Nov 5, 2015

@yiminghe Maybe I'm missing something, but isn't flattenChildren also now being called in mountWrapper?

@facebook-github-bot
Copy link

@yiminghe updated the pull request.

@yiminghe
Copy link
Contributor Author

yiminghe commented Nov 5, 2015

@jimfb add a flag to only warn in getNativeProps

https://github.com/yiminghe/react/blob/initial-option-select/src/renderers/dom/client/wrappers/ReactDOMOption.js#L20

@jimfb
Copy link
Contributor

jimfb commented May 3, 2016

@yiminghe Sorry we dropped the ball on this one, but I'd like to try to get this merged. Can you rebase and add a unit test to demonstrate that it only warns once, and then I think we can merge.

@gaearon gaearon mentioned this pull request May 3, 2016
Closed
@sophiebits
Copy link
Collaborator

Can you set a global (file-level) flag so that we only warn once, even if many children are the wrong type? Because we don't give information about the callsite it's not currently useful to warn many times.

A similar example is here:

react/src/renderers/dom/client/wrappers/ReactDOMTextarea.js

Line 43 in 1d49baf

didWarnValueNull = true;

Then you don't even need the needWarning parameter.

@yiminghe
Copy link
Contributor Author

yiminghe commented May 9, 2016

updated as requested

gaearon
gaearon reviewed May 9, 2016
View reviewed changes
src/renderers/dom/client/wrappers/ReactDOMOption.js
}
if (typeof child === 'string' || typeof child === 'number') {
content += child;
} else if(!didWarnInvalidOptionChildren) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor nit: missing space before (

@facebook-github-bot
Copy link

@yiminghe updated the pull request.

@sophiebits sophiebits merged commit 904ee9a into facebook:master May 9, 2016
@sophiebits
Copy link
Collaborator

Thank you!

@zpao zpao added this to the 15.y.z milestone May 17, 2016
zpao pushed a commit to zpao/react that referenced this pull request Jun 8, 2016
@yiminghe @zpao
allow to ignore value attribute for option (facebook#5362)
8d633a5 
(cherry picked from commit 904ee9a)
zpao pushed a commit that referenced this pull request Jun 14, 2016
@yiminghe @zpao
allow to ignore value attribute for option (#5362)
818ef42 
(cherry picked from commit 904ee9a)
@zpao zpao modified the milestones: 15-next, 15.2.0 Jun 14, 2016
@renovate renovate bot mentioned this pull request Feb 2, 2018
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
CLA Signed HTML
Projects
None yet
Milestone
15.2.0
Development

Successfully merging this pull request may close these issues.
7 participants
@yiminghe @facebook-github-bot @jimfb @sophiebits @gaearon @zpao @sebmarkbage