Skip to content

Add didLeave method to <TransitionMotion /> - #399

Merged
chenglou merged 1 commit into
chenglou:masterfrom
hkjorgensen:hkjorgensen/add_didLeave_to_transitionmotion
Dec 7, 2016
Merged

Add didLeave method to <TransitionMotion />#399
chenglou merged 1 commit into
chenglou:masterfrom
hkjorgensen:hkjorgensen/add_didLeave_to_transitionmotion

Conversation

@hkjorgensen

Copy link
Copy Markdown
Contributor

Had a case where the index of each element was essential for correct visual representation. didLeave can be used to keep track of what elements <TransitionMotion /> is still animating.

Can also be used as the onRest equivalent for <TransitionMotion />.

@hkjorgensen
hkjorgensen force-pushed the hkjorgensen/add_didLeave_to_transitionmotion branch 2 times, most recently from 2d9a650 to 3b22bca Compare November 26, 2016 08:22
@hkjorgensen
hkjorgensen force-pushed the hkjorgensen/add_didLeave_to_transitionmotion branch from 3b22bca to 453e6d6 Compare November 26, 2016 08:25
@hkjorgensen

Copy link
Copy Markdown
Contributor Author

@nkbt @chenglou do one of you have time to take a look?

@chenglou

chenglou commented Dec 6, 2016

Copy link
Copy Markdown
Owner

Will do tonight!

@chenglou

chenglou commented Dec 6, 2016

Copy link
Copy Markdown
Owner

Alright, this looks good. Tbh I've forgotten a big chunk of the codebase. Since the tests pass this is non-breaking, I can merge it. But before that, are we sure about the name here? DidLeave vs onRest?

@souporserious

souporserious commented Dec 6, 2016

Copy link
Copy Markdown
Contributor

I like onRest to match the Motion component.

So excited for this! This will allow me to do auto height animation in the UI pack 🙌

@DetweilerRyan

Copy link
Copy Markdown

didLeave seems to be a more appropriate name than onRest. To me at least, onRest implies that the TransitionMotion component as a whole is no longer in motion. While, on the other hand, didLeave implies that a single child has finished transitioning out of the TransitionMotion component, yet other children may still be in motion.

@hkjorgensen

Copy link
Copy Markdown
Contributor Author

didLeave seems to be a more appropriate name than onRest. To me at least, onRest implies that the TransitionMotion component as a whole is no longer in motion. While, on the other hand, didLeave implies that a single child has finished transitioning out of the TransitionMotion component, yet other children may still be in motion.

@DetweilerRyan, that was also what I thought when naming it 'didLeave' :-)
onRest would be misleading compared with <Motion onRest />.
Next, I would suggest/PR for a didEnter, and those two combined would allow for a proper onRest implementation.

@chenglou do you see react-motion as abandonware? I still believe it's one of the best packages for doing spring animations in React. The spring logic is solid, but there is always room for further development for the API :-)

@chenglou

chenglou commented Dec 7, 2016

Copy link
Copy Markdown
Owner

I'm aggressively cautious about adding API overhead. TransitionMotion's API has a bit of learning overhead already and I didn't want to add more to it. Lots of people add an animation to their project then move on, so having a stable API throughout time in a (whether I want it or not) flagship react project might be worth more than finding a neater API that breaks dependents every few months. Just doing my part of not adding churn =)

Personally, I'm overwhelmed with emails lately so even pings against me might go unnoticed. Trying to catch up though. Also I've been working on some important react-related work lately too (will be open-sourced this week. It's gonna change everything!). But I have to apologize for the codebase quality too. It's not exactly straightforward to navigate it I feel. Feel free to submit more PRs though! And ping me about them!

Thanks a lot for this PR. I'll be merging this. Do we want to make a new release?

@chenglou
chenglou merged commit bdfef4e into chenglou:master Dec 7, 2016
@souporserious

Copy link
Copy Markdown
Contributor

Yes to a new release please 🙏 thanks for this @hkjorgensen!

@hkjorgensen

hkjorgensen commented Dec 12, 2016

Copy link
Copy Markdown
Contributor Author

@chenglou Thanks for the explanation - I still look forward to that new open source project :-)

Thanks a lot for this PR. I'll be merging this. Do we want to make a new release?

Yes, please cut a new version. This expands the API and doesn't break anything :-)

@chenglou

Copy link
Copy Markdown
Owner

Ok, published as a patch update then! Tell me if anything goes wrong, thanks.

therewillbecode pushed a commit to therewillbecode/react-motion that referenced this pull request Mar 3, 2017
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.

4 participants