Skip to content

Compatible with ReactTransitionGroup? #727

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

Closed
klase opened this issue Jan 26, 2015 · 38 comments
Closed

Compatible with ReactTransitionGroup? #727

klase opened this issue Jan 26, 2015 · 38 comments

Comments

@klase
Copy link

klase commented Jan 26, 2015

Does React Router work with ReactTransitionGroup? Maybe I'm missing something but no matter what I try the lifecycle hooks won't trigger on components associated with a route: http://jsfiddle.net/d2mnwqLj/3/

@gaearon
Copy link
Contributor

gaearon commented Jan 26, 2015

I think @natew may have expertise related to this.
See also: #543

Please let me know if this helps (or not)!

@klase
Copy link
Author

klase commented Jan 26, 2015

Seems to be the same issue. I was going to try the The RouteHandler mixin but it doesn't seem to be publicly exposed in the bower build?

@gaearon
Copy link
Contributor

gaearon commented Jan 26, 2015

It should be.

@mjackson
Copy link
Member

We really, really need to cut a release. Master is too far ahead of 0.11.

@klase
Copy link
Author

klase commented Jan 26, 2015

It's not exposed in the global build on cdnjs which I was using. I'll try compiling myself but need to get my head around webpack first..

@gaearon
Copy link
Contributor

gaearon commented Jan 26, 2015

@klase You should be able to compile with npm install && npm build-min.

@klase
Copy link
Author

klase commented Jan 26, 2015

Thanks that worked like a charm. Still not able to solve the original issue though, the RouteHandlerMixin doesn't have the getRouteHandler function referred to in #543 ? Do I need to grab it from the pull request #559 perhaps?

@gaearon
Copy link
Contributor

gaearon commented Jan 26, 2015

@klase I assume it is now called createChildRouteHandler.

@klase
Copy link
Author

klase commented Jan 26, 2015

Cheers the lifecycle hooks now trigger but I get this warning every time I hit a route: "Warning: You are calling cloneWithProps() on a child with a ref. This is dangerous because you're creating a new child which will not be added as a ref to its parent."

@zkilgore
Copy link

zkilgore commented Feb 3, 2015

@ryanflorence @klase I'm seeing this warning as well when I use the same implementation. Possible issue with react version we're using?

@zkilgore
Copy link

@digitaltoad @alexaivars I was working through this last night, and this is a basic implementation that works, albeit with a caveat I'll get to:

var TransitionGroup = React.addons.TransitionGroup;
var {RouteHandlerMixin, State} = require("react-router");

module.exports = React.createClass({

  mixins: [ RouteHandlerMixin, State ],

  getHandlerKey: function() {
    return this.getRoutes().reverse()[0].name;
  },

  render: function() {
    var key = this.getHandlerKey();
    return(
      <TransitionGroup>
        {this.createChildRouteHandler({key: key})}
      </TransitionGroup>
    );
  }
});

However with this implementation you lose the ability to access the component argument of the static lifecycle method willTransitionFrom on the handler. This is because the RouteHandlerMixin mixes in responsibility for setting the component the method is passed with _updateRouteComponent when the handler mounts/updates. It tries to access the component that was created via the call to createChildRouteHandler via this.refs[REF_NAME] ([REF_NAME is just a constant](https://github.com/rackt/react-router/blob/master/modules/RouteHandlerMixin.js#5)). The problem is that React TransitionGroup blows this ref away internally as it takes ownership away from the RouteHandlerMixin'd component because of it's use of cloneWithProps`.

I spent a bit coming up with a hacky solution that passes the _updateRouteComponent function as a prop to a modified TransitionGroup so that the TransitionGroup component updates the handler component on it's mount/update. If anyone is interested or needs this piece I can try and clean it up and post.

@LKay
Copy link

LKay commented Mar 9, 2015

Doesn't seem to be working. I tried the code above as well as using default handler and not the one from mixin. When the route is changing after calling transitionTo the content of the div which holds page content gets replaced and elements for transitions get duplicated. http://cl.ly/image/2T3w3H3r3m39

I want to keep the content of the previous rout untouched until transition to the new route ends. Is that possible?

@natew
Copy link
Contributor

natew commented Mar 9, 2015

@zkilgore In react 0.13 they added a new cloneElement that doesn't blow away the ref. Perhaps you could try patching TransitionGroup?

@zkilgore
Copy link

@natew Thank you for the tip! Will spend some time upgrading soon and report back how it goes. I'm excited about cloneElement.

@LKay are you implementing componentWillEnter and componentWillLeave on your handlers?

@LKay
Copy link

LKay commented Mar 12, 2015

@zkilgore I tried to implement these methods in the first handler for the route but the elements still get duplicated when transition starts.

@zkilgore
Copy link

@LKay can you toss what you have in a gist and post it?

@duro
Copy link

duro commented Mar 20, 2015

@LKay I'm also very interested in seeing this. I was able to get ReactCSSTransitionGroup to work, but for the life of me ReactTransitionGroup does not work.

@natew
Copy link
Contributor

natew commented Mar 20, 2015

Are you guys using the mixin rather than the <RouteHandler /> component?

I'm not using TransitionGroup directly, but I have a component that is basically a re-implementation of it and it works well once you get rid of the component.

@duro
Copy link

duro commented Mar 20, 2015

@natew Can you send a gist of your implementation?

@duro
Copy link

duro commented Mar 20, 2015

@natew And are you using React 0.13 or 0.12?

@natew
Copy link
Contributor

natew commented Mar 20, 2015

@duro it would be hard to extract, but the usage is in reapp rather than TransitionGroup, and the ViewList's use the same ideas as transitiongroup (https://github.com/reapp/reapp-ui/blob/master/mixins/ViewListMixin.jsx#L96) but to be honest you guys are probably further into this than me as far as using transitiongroup.

For me I got transitions working only with the mixin, but both on 0.12 and 0.13.

@zkilgore
Copy link

@duro here's a gist of pretty much what I'm doing (albeit faked). But these are the methods I implement on my handlers to make animations work.

https://gist.github.com/zkilgore/254307f7cd5c42579ed0

And I'm still on 0.12 but think it should be fine in 0.13 too.

@duro
Copy link

duro commented Mar 20, 2015

@zkilgore Do you get this warning in your implementation:

Warning: You are calling cloneWithProps() on a child with a ref. This is dangerous because you're creating a new child which will not be added as a ref to its parent.

@zkilgore
Copy link

I do get that, but it's not a huge deal, and though I haven't implemented
0.13 yet I know that there is a new clone method called cloneElement which
from what I've read will alleviate that.

One caveat is that you won't be able to use the component in the transition
hooks unless you modify the transition group code. I wrote about it in a
previous comment, and if that's an issue for you I can post something on
that too. Only reason I'm hesitant is that I did it pretty quickly in the
first place and was gonna refactor it soon anyway...
On Fri, Mar 20, 2015 at 7:37 PM Adam Duro [email protected] wrote:

@zkilgore https://github.com/zkilgore Do you get this warning in your
implementation:

Warning: You are calling cloneWithProps() on a child with a ref. This is dangerous because you're creating a new child which will not be added as a ref to its parent.


Reply to this email directly or view it on GitHub
#727 (comment).

@duro
Copy link

duro commented Mar 21, 2015

@zkilgore Yea, I have tested going back and forth between TransitionGroup and CSSTransitionGroup. When I use CSSTransitionGroup I have no problem, as soon as I switch to TransitionGroup I start getting that warning and things start breaking.

Switching between basic routes is fine. But if I try to switch to a Handler that expects props to merge down from the RouteHandler, that Handler never gets the props.

My main question here is, what is broken? React-Router? Or TransitionGroup? It seems like we are hacking around some incompatibility here, and I'm wondering who's incompatibility I should be focusing on.

@zkilgore
Copy link

@duro can you post a gist of what you're working with in the case where it
breaks?

I don't think either of these components are broken (in fact they're both
awesome), but the way they use refs and keys and the fact that transition
group clobbers the handlers ref makes it a challenge. Take a look at the
source of these to see what I mean, they're both straightforward.
On Fri, Mar 20, 2015 at 9:14 PM Adam Duro [email protected] wrote:

@zkilgore https://github.com/zkilgore Yea, I have tested going back and
forth between TransitionGroup and CSSTransitionGroup. When I use
CSSTransitionGroup I have no problem, as soon as I switch to
TransitionGroup I start getting that warning and things start breaking.

Switching between basic routes is fine. But if I try to switch to a
Handler that expects props to merge down from the RouteHandler, that
Handler never gets the props.

My main question here is, what is broken? React-Router? Or
TransitionGroup? It seems like we are hacking around some incompatibility
here, and I'm wondering who's incompatibility I should be focusing on.


Reply to this email directly or view it on GitHub
#727 (comment).

@LKay
Copy link

LKay commented Mar 21, 2015

@zkilgore @duro
I was able to fix it somehow but I don't really know what caused the problem with cloning content of the component. The base structure gist is here https://gist.github.com/LKay/babc1186f9e37c3a04f0

I was not using Mixin but just <RouteHandler />, React 0.12 and have nested handlers in corresponding routes. The problem in my case was mainly because states changing in Flux structure in my app and it messed somehow with RouteHandler. I cleaned this up a little and problem disappeared.

@duro
Copy link

duro commented Mar 21, 2015

@LKay your gist appears to be implementing CSSTransitionGroup and not the lower level TransitionGroup. Are you implementing componentWillEnter and componentWillLeave life cycle methods on your RouteHandlers?

@duro
Copy link

duro commented Mar 23, 2015

@zkilgore Here is a gist of the files in question:

https://gist.github.com/duro/0d6d3ef4d5b53be5e1e7

The main problem seems to stem from views (aboutServiceCategory.js) that need to load data via a static method before they are rendered. Other pages (home.js, careers.js, aboutLanding.js) that don't have bootstrapped data work fine.

I understand that individually both these libraries kick ass. It's just the incompatibility between the two that has me frustrated.

Any ideas on what I may be doing wrong?

@zkilgore
Copy link

@duro Is the problem that when you switch between the 'about service category' pages the transition lifecycle methods are not called? If that's the issue, the problem is the handler will have the same key (the leaf route name, so 'serviceDetail') and in the TransitionGroup's eyes that's component isn't leaving or entering. You could append the :category param to the key to combat this.

If that is not the issue, then could you describe what exactly you want to happen and what exactly is happening?

@duro
Copy link

duro commented Apr 1, 2015

I have upgraded everything to 0.13, and my problem now seems to be consistent. Props are not being passed down to newly created route handlers. If I move from a RouteHandler that needs to props, to a RouteHandler that needs props, the transition does not work.

@colinramsay
Copy link

I managed to get this to work, finally:

https://gist.github.com/colinramsay/42d3dbd0316212d65161

@duro
Copy link

duro commented May 12, 2015

I'll test this ASAP. If this works you are a God in my book.

On Tuesday, May 12, 2015, Colin Ramsay [email protected] wrote:

I managed to get this to work, finally:

https://gist.github.com/colinramsay/42d3dbd0316212d65161


Reply to this email directly or view it on GitHub
#727 (comment).

@colinramsay
Copy link

Note that I might not have had exactly the same issue as you, this is just super-basic usage. The whole discussion of React and React Router animations really needs some TLC at this point I think.

@gaearon
Copy link
Contributor

gaearon commented May 12, 2015

You might want to try the next branch, as it's API is different, and there's no RouteHandler anymore (children are injected as normal props). I don't know if it makes animations simpler, but it might.

@duro
Copy link

duro commented May 19, 2015

@colinramsay I took a deeper look at your component. It seems to implement a similar approach to ReactCSSTransitionGroup which is already in the React core. I was always able to get that one working without problem. My challenge was getting the componentDidEnter and componentWillLeave to fire on the RouteHandlers themselves. This way I can use pure JS animations like the GreenSock animation library.

@gaearon Are there any upgrade notes for the next branch?

@duro
Copy link

duro commented Jun 15, 2015

@gaearon I see the next branch is now deleted? Did the rewrite get merged, or was it discontinued?

@srph
Copy link

srph commented Jun 15, 2015

It was merged.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants