fix option update. Fixes #2620 #3817
Conversation
| function getChildrenTextContent(tag, children) { | ||
| var childrenContent = ''; | ||
| ReactChildren.forEach(children, function (c) { | ||
| if ("production" !== process.env.NODE_ENV) { |
There was a problem hiding this comment.
please use if(__DEV__) instead of "production" !== process.env.NODE_ENV
|
@yiminghe Overall looks good. I'm excited to see this fix! A couple of comments. Also, the linter is unhappy (you can execute |
|
Tracking issue in #2620 |
| @@ -23,6 +23,8 @@ var ReactComponentBrowserEnvironment = | |||
| var ReactMount = require('ReactMount'); | |||
| var ReactMultiChild = require('ReactMultiChild'); | |||
| var ReactPerf = require('ReactPerf'); | |||
| var ReactChildren = require('ReactChildren'); | |||
| var ReactElement = require('ReactElement'); | |||
There was a problem hiding this comment.
nit: abc sort the requires (there's no lint rule for this)
|
This logic would actually be better in ReactDOMOption especially since we already have that wrapper, not in ReactDOMComponent. |
| @@ -775,4 +775,39 @@ describe('ReactDOMComponent', function() { | |||
| }); | |||
| }); | |||
|
|
|||
| describe('option update', function(){ | |||
| var container = document.createElement('div'); | |||
There was a problem hiding this comment.
These should be in a beforeEach so that the container is reinitialized before each spec.
|
This is not specific for option (maybe in the future), that's why i add onlyNestTextTags map. |
|
I would be far less concerned if we simply threw on multiple children, or no longer made |
|
@yiminghe I understand. I'd prefer to have the logic in ReactDOMOption because I don't anticipate us adding it for other tags. If we choose to do that in the future, we can look at abstracting it out – but ReactDOMComponent is already very complicated and so we'd like to avoid complicating it unnecessarily. Can you move the code to ReactDOMOption's render function? |
| var childrenContent = ''; | ||
| ReactChildren.forEach(children, function (c) { | ||
| var childType = typeof c; | ||
| if (childType !== 'string' && childType !== 'number') { |
There was a problem hiding this comment.
This if statement would be clearer with the branches swapped.
|
Actually, #3847 looks closer so I'll follow up with that one instead. Thanks for sending this in. |
Option update will fail if option's content is mixed with variable and literal text. for example:
https://jsfiddle.net/yiminghe/jw73ccu5/1/