Skip to content

Animations not working #543

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
natew opened this issue Nov 29, 2014 · 29 comments
Closed

Animations not working #543

natew opened this issue Nov 29, 2014 · 29 comments

Comments

@natew
Copy link
Contributor

natew commented Nov 29, 2014

I know this is commented in the animations demo but just opening a discussion here. I've been playing with this a little this morning and not having luck using plain TransitionGroup (not CSSTransitionGroup) and can never get the componentWillEnter to call.

I've struggled to get non-CSS TransitionGroups working before though, so wanted to open this here first before going too far into it.

@ryanflorence
Copy link
Member

can you get it working with elements that have nothing to do with the router?

@natew
Copy link
Contributor Author

natew commented Nov 29, 2014

Made a small test for both CSS and non-CSS and was unsuccessful with both... are the React addon transitions no longer working?

@natew
Copy link
Contributor Author

natew commented Nov 29, 2014

Ah ok, perhaps something to do with the new noscript behavior. I can get CSS ones working if I completely avoid rendering inner contents.

@acdlite
Copy link
Contributor

acdlite commented Nov 29, 2014

Hmm it's working for me. Are you adding keys to the elements inside the transition group?

@natew
Copy link
Contributor Author

natew commented Nov 29, 2014

Ah figured it out. Using regular transitiongroups is breaking. If I add to RouteHandler:

componentWillEnter() {
  debugger;
},

It will pause, but if I have that method in a child component, it doesn't. Perhaps I could avoid this by having a wrapper component above RouteHandler though.

@skevy
Copy link

skevy commented Jan 15, 2015

@natew any luck on this? I'm curious if anyone has fixed this. I have some ideas on how I might, but I don't want to do it if I don't have to.

I need componentWillEnter, componentDidEnter, etc to be passed down. They currently are not.

I tried making my AppHandler use RouteHandlerMixin and then do: "{this.getRouteHandler(_.assign({}, this.props, { key: routeName }))}" instead of , and it works...but then things like willTransitionFrom, willTransitionTo doesn't work.

Ideas?

@natew
Copy link
Contributor Author

natew commented Jan 15, 2015

@skevy yea this actually is still an issue if you use the RouteHandler component. I actually ended up submitting a pull request (#559) that moved most of RouteHandler into a mixin (mixins/RouteHandler). You can see in that pull request how it allows you to avoid using the RouteHandler component.

So instead you use this.getRouteHandler() in your parent, and your child now receives props passed down correctly.

I haven't tested this with animations fully, I actually ended up using a different strategy that avoids TransitionGroup (but basically does the same thing as it does, just as a mixin as well in my classes).

@skevy
Copy link

skevy commented Jan 16, 2015

@natew So I actually did the this.getRouteHandler thing, but do you know if it works with the statics (like willTransitionFrom)?

@skevy
Copy link

skevy commented Jan 16, 2015

I'm probably just gonna fix this...on a tight deadline and don't have time to wait on things from this project (as much as I love it and everyone working on it)...but if it's actually good code, I'll send a pull request. But at the very least, I'll update this ticket.

@natew
Copy link
Contributor Author

natew commented Jan 16, 2015

Should work with statics, because the child route is given to you directly.

this.getRouteHandler().type.XXX

Does that work? Sorry, haven't touched this code in a while so my memory's fuzzy on it. Let me know what you come up with.

@mjackson
Copy link
Member

@natew The best way I can think of to solve this issue is to just get rid of <RouteHandler> altogether and require people to mix in Router.RouteHandler to their route handler components. It's kind of clunky tho...

Is there a better way?

@gaearon
Copy link
Contributor

gaearon commented Jan 22, 2015

I actually kind of like that change, if it were implemented. Makes everything a bit less magical.

@mjackson
Copy link
Member

@gaearon is there a way we could automatically mix in RouteHandler to people's route handler components? or at least warn them when they don't do it themselves? i just want to avoid the situation where someone is staring at a blank screen, wondering why nothing is rendered, because they forgot the mixin.

@natew
Copy link
Contributor Author

natew commented Jan 22, 2015

Thats what I'm doing now, mixing it in and using this.getRouteHandler(). It works as of today actually.

It feels better in some ways in that it breaks less things, but maybe isn't as simple and consistent with React syntax. I like it overall though.

One thing is passing down props you don't get the JSX syntax:

var RouteHandler = require('react-router/modules/mixins/RouteHandler');

var Component = React.createClass({
  mixins: [RouteHandler],
  render() {
   return this.getRouteHandler(Object.assign(this.props, { key: myKey }));
  }
});

But to me, thats a really minor trade-off.

Edit: I guess you could do this, no? Perhaps not, the new JSX changes still haven't stuck in my head.

var Handler = this.getRouteHandler();
return <Handler {...this.props} />;

@natew
Copy link
Contributor Author

natew commented Jan 22, 2015

Would be really nasty, but react-router could inject createClass and make this.getRouteHandler log out a warning that it needs a mixin? I feel icky typing that though :)

@mjackson
Copy link
Member

@natew wait a sec...

Isn't that what ReactInjection.Class.injectMixin is for?!

We could just put that in and be done with it.

@natew
Copy link
Contributor Author

natew commented Jan 22, 2015

Honestly never used it, @gaearon or @petehunt probably know a lot more in this area.

@mjackson
Copy link
Member

Ya, it's probably not a great idea because we'd be adding the mixin to every single class that way...

I guess we'll just need to keep the separate mixin for people who want to do fancy stuff, like animations. We should def document it tho.

@gaearon
Copy link
Contributor

gaearon commented Jan 22, 2015

injectMixin would put it into every class so it's not an option.

Edit: I guess you could do this, no? Perhaps not, the new JSX changes still haven't stuck in my head.

var Handler = this.getRouteHandler();
return <Handler {...this.props} />;

Indeed, this should work.

I agree “mixin for special cases” seems good if it is documented.

@gaearon
Copy link
Contributor

gaearon commented Jan 22, 2015

is there a way we could automatically mix in RouteHandler to people's route handler components?

Once class is created, you can't inject a mixin into it, so no.

@ryanflorence
Copy link
Member

sounds like we just need docs here, no action items until somebody has a brilliant idea.

@digitaltoad
Copy link

Are there any solid answers to this problem yet? I'm still not exactly sure how to use the RouteHandlerMixin in a way that allows children to receive the TransitionGroup hooks.

@alexaivars
Copy link

+1 (really confused about RouteHandlerMixin and the new createChildRouteHandler) love to se some docs or updated animation example.

@greypants
Copy link

+1

@kilometers
Copy link

+1

@natew
Copy link
Contributor Author

natew commented Apr 14, 2015

Reapp does this. Basically you extract most stuff from RouteHandler into a mixin:

https://github.com/reapp/reapp-routes/blob/master/src/react-router/ParentRouteMixin.js

Then I made a new class called a React.Page:

https://github.com/reapp/reapp-kit/blob/master/src/lib/Page.js

(Note it mixes in RoutedViewListMixin which is just ParentRouteMixin with some Reapp helpers that automatically set properties needed for animations when routes change).

I'd like to write more on how this all works, just slammed with client work atm.

@duro
Copy link

duro commented Apr 29, 2015

I would also love to get some clairity on how to best implement this. While I appreciate @natew 's example, it doen't quite provide the clairty I'm looking for. Does anyone have a simple example?

@duro
Copy link

duro commented Apr 29, 2015

It seems the main problem here is that the RouteHandlerMixin is now removed from the react-router project. So has the one solution that people were claiming to work been abandoned? Is it safe to say that react-router is simply NOT compatible with ReactTransitionGroup?

@ryanflorence
Copy link
Member

1.0 now uses props for child route components, so animating with react router is identical to animating with any other component.

@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