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

ReactTransitions: Don't animate undefined children #537

Merged
merged 2 commits into from
Dec 1, 2013
Merged

ReactTransitions: Don't animate undefined children #537

merged 2 commits into from
Dec 1, 2013

Conversation

hojberg
Copy link
Contributor

@hojberg hojberg commented Nov 14, 2013

This fixes: #521

@@ -44,6 +44,11 @@ var ReactTransitionGroupMixin = {
'getTransitionConfig() method.'
);

// don't animate undefined children
if (typeof sourceChildren === 'undefined') {
Copy link
Contributor

Choose a reason for hiding this comment

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

how about nulls or empty arrays? may be cool to get the keySet and check Object.keys(keySet).length === 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Object.keys(undefined) throws (which is the exception i'm trying to avoid).

ReactChildren.map(undefined, fn) will return undefined, which will bubble up, until it meets mergeKeySets which uses Object.keys.

We could let mergeKeySets handle the undefined inputs?

@sophiebits
Copy link
Collaborator

I believe this won't properly do the transitions when transitioning away from undefined children. You instead want to treat undefined effectively the same as an empty list -- perhaps just make the change where the Object.keys call is?

Gracefully handle undefined input to mergeKeySet.
@hojberg
Copy link
Contributor Author

hojberg commented Nov 14, 2013

@spicyj @petehunt changed to handle undefined children closer to the exception, in mergeKeySets.

Let me know if you'd like this handled elsewhere.

@ghost ghost assigned petehunt Nov 18, 2013
@hojberg
Copy link
Contributor Author

hojberg commented Nov 19, 2013

@spicyj @petehunt any changes needed here or good to merge?

@sophiebits
Copy link
Collaborator

This looks fine to me. The codepath is already O(n^2) so I suspect allocations aren't a huge deal.

@hojberg
Copy link
Contributor Author

hojberg commented Nov 25, 2013

@spicyj @petehunt any update on merging this?

@petehunt
Copy link
Contributor

petehunt commented Dec 1, 2013

I'll be merging this this week! Sorry for the delay!

@petehunt
Copy link
Contributor

petehunt commented Dec 1, 2013

And by this week I mean today

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

React.addons.TransitionGroup should not throw on undefined children
3 participants