-
Notifications
You must be signed in to change notification settings - Fork 48.3k
ReactTransitionGroup: fix fast enter/leave issue #5028
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
ReactTransitionGroup: fix fast enter/leave issue #5028
Conversation
This fixes bugs when components are remove from a `ReactTransitionGroup` before they finish entering or added back before they finish leaving.
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks! |
I'll wait to alter the tests until I've heard back that the issue will be accepted...it's proving a bit difficult to get the build working on my computer |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
Any progress on this PR? |
If this fixes issues like #5575 maybe we can get this in. Nobody had a chance to review this yet so I want to give it a try. Sorry it took so long. The problem is it’s not immediately clear (at least to somebody like me who isn’t intimately familiar with how TransitionGroup works internally) how this reflects on the real apps. I looked at your test case in https://github.com/jedwards1211/react/blob/react-transition-group-patch/ but I found the test case a little too abstract. It would help if you tweaked it to look more like a problem a real app can face with some common UI pattern (e.g. an accordion). Otherwise it’s hard to tell if this is a bugfix or a feature request for different behavior. Also lack of tests makes it hard to tell whether this is going to be a breaking change. Is old behavior undisputedly a bug, or can people actually be relying on it? What about people who use |
Sorry, I haven't looked at this in a long time. So I wish this would be considered a bug, but technically it's more like a request for different behavior. I'm basically trying to argue that most users would probably find it least surprising and most helpful if ReactTransitionGroup and ReactCSSTransitionGroup behave the same way as CSS transitions do when the transitioning property is changed in the middle of a transition. In that case CSS will interrupt the transition in progress and begin a new transition to the new value. ReactTransitionGroup doesn't interrupt a transition in progress; this PR makes it interrupt a leave transition if the component re-enters or interrupt an enter transition of the component leaves. Does that make sense? I'm sure that part of the reason the tests are failing is that this PR changes the semantics to make transitions interruptible. Would you like for me to go ahead and alter the tests/write more tests for these new semantics? Or wait until the semantics changes are approved? |
We’re not in a very good state right now regarding changes to TransitionGroup due to the reasons I explained in #6149 (comment). Unfortunately I can’t promise anything right now other than we will discuss the plan for TransitionGroup this week and come back with some plan moving forward after shipping React 15. I think that it would help strengthen your case if you added tests but I can’t guarantee we’ll take this PR. Sorry for being inconclusive. This is something we should get better at. Thank you very much for contributing. |
Okay, well it's no big deal for me as I already use this and a version of ReactCSSTransitionGroup built on top of it in my own projects anyway, I just thought I would share this. Perhaps at some point I should release my versions as a library. |
I also ran into this issue (#5027), particularly leave being delayed by enter. @gaearon Thanks for explaining the state of this addon, but would it be useful to make a note of it in documentation? Also, do you have any recommendations for alternatives to ReactTransitionGroup, or perhaps some context on what you use internally? |
I would suggest trying out https://github.com/chenglou/react-motion. |
We don’t have an owner for ReactTransitionGroup internally because we don’t use it, and it seems like we don’t have the capacity to review PRs for it at the moment. Your best strategy might be to fork its code (or find an existing fork) and propose these changes there. If any of the forks turns out to be successful we might deprecate our own version and start pointing to the fork in the docs. Sorry it turned out to be this way, but it’s best to do something than to leave this hang forever so I’m closing. |
This fixes #5027 by making entering/leaving transitions interruptible, just like CSS:
componentWillLeave
is called immediatelycomponentWillEnter
is called immediatelycomponentWillAppear
calls its callback, no action is taken unless the component is in appearing state.componentWillEnter
calls its callback, no action is taken unless the component is in entering state.componentWillLeave
calls its callback, no action is taken unless the component is in leaving state.I haven't yet edited the tests to reflect the new behavior, work in progress.