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

Fix errors in React Native children management #9449

Merged
merged 2 commits into from
Apr 19, 2017

Conversation

sophiebits
Copy link
Collaborator

  • Support using appendChild to move an existing child (Fiber does this, but we were assuming all children here were new; Yoga throws if you insert a child that already has a parent)
  • Calculate beforeChildIndex after removing old child (previously, off by one when the new position is later than the old position)

- Support using appendChild to move an existing child (Fiber does this, but we were assuming all children here were new; Yoga throws if you insert a child that already has a parent)
- Calculate beforeChildIndex after removing old child (previously, off by one when the new position is later than the old position)
@sophiebits
Copy link
Collaborator Author

Tests to come.

@bvaughn bvaughn self-assigned this Apr 18, 2017
Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

Nice catch, Ben.

Support using appendChild to move an existing child (Fiber does this, but we were assuming all children here were new; Yoga throws if you insert a child that already has a parent)

I would not have expected appendChild to be called for an already-appended child. I wonder if this will cause problems for other custom renderers?

A test or two would be nice! Otherwise LGTM

@sophiebits
Copy link
Collaborator Author

I can't figure out how to test the append thing easily but my test definitely covers the other one (adapted from a React ART test).

@sophiebits sophiebits merged commit b392f1e into facebook:master Apr 19, 2017
@gaearon
Copy link
Collaborator

gaearon commented Apr 19, 2017

Thanks for adding the test.

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.

None yet

4 participants