-
Notifications
You must be signed in to change notification settings - Fork 47.1k
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
Remove content property #120
Conversation
Welp, I wasn't expecting anybody to go for it yet, so bravo! We're going to have to do some big changes internally before we can merge this in (since we're still using |
:) Figured that might be the case. (I'm just creating merge conflicts for myself with my textarea pull request anyway. Ah well.) |
Just saw this, haha. Bravo @spicyj. |
hasChildren + hasContent + hasInnerHTML <= 1, | ||
'Can only set one of `children`, `props.content`, or ' + | ||
'`props.dangerouslySetInnerHTML`.' | ||
hasChildren + hasInnerHTML <= 1, |
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.
Let's change this to just check props.children
and props.dangerouslySetInnerHTML
since we only have the 2 things to check.
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.
Do you mean, get rid of the named vars?
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.
Yea, we don't need to do any math to make sure you don't have both of these props. Math worked better than straight logic once you have 3.
(simplified invariant check, added link to jsperf for emptying a node) |
Update selected index after mutation
Fixes #119.