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

Is offload animation ready for navigator? #9421

Closed
narychen opened this issue Aug 16, 2016 · 20 comments
Closed

Is offload animation ready for navigator? #9421

narychen opened this issue Aug 16, 2016 · 20 comments
Labels
Ran Commands One of our bots successfully processed a command. Resolution: Locked This issue was locked by the bot.

Comments

@narychen
Copy link
Contributor

It's been a long while since the production pain has been launched.
I saw there were both ios and android PR has been merged.
But the official doc says nothing about it. The performance bottleneck of navigator is still there.

@DanielMSchmidt
Copy link
Contributor

I think there was a talk about this at the last react-conf and I think there were also some mentions in this video from the react-europe.

What would you like to see included in the docs?

@DanielMSchmidt
Copy link
Contributor

@facebook-github-bot label Documentation

@ghost ghost added Documentation Ran Commands One of our bots successfully processed a command. labels Aug 17, 2016
@narychen
Copy link
Contributor Author

@DanielMSchmidt The first video you gave is too early to be related to what I'm talking about.
The second video is all about the JIT of react-native. So I think you might be mis-understanding what I mean of offloading animations

@DanielMSchmidt
Copy link
Contributor

Yes, seems so, sorry about that

@satya164
Copy link
Contributor

We also need to move to Animated.Event instead of calling setValue directly in NavigationExperimental. Would be great if someone can send a PR for that.

@narychen
Copy link
Contributor Author

@satya164 Does the old navigator has some plan of it ? Since a lot of project use navigator already rather than NavigationExperimental. It's a lot of work to migrate to the new one.

@satya164
Copy link
Contributor

@narychen I think the old navigator doesn't use Animated. So it won't be possible to use native animations there.

@narychen
Copy link
Contributor Author

@satya164 Does the new NavigationExperimental enable offloading animation ?

@satya164
Copy link
Contributor

It doesn't now. But since it uses Animated, it'll be possible once the work on native animations is complete.

@narychen
Copy link
Contributor Author

@satya164 Will the NavigationExperimental be the official support of navigation? Should I migrate all my navigator code to NavigationExperimental?

@satya164
Copy link
Contributor

@narychen Yes, it'll be the officially supported way going forward. If you're migrating to NavigationExperimental, keep in mind that while it's pretty stable as of now, there are small API changes here and there every release.

@nickhudkins
Copy link
Contributor

I'd love to see this move forward. What can I do to help out here? Has work begun to use Animated.Event rather than setValue?

@ptomasroos
Copy link
Contributor

Android Animated.Event has landed in master but IOS is still waiting in line being pulled. But you should def be able to run native offloading and Android and fork against migrating setValue to Animated.Event

#9253
#9598

@nickhudkins
Copy link
Contributor

To make sure I understand what is between the current implementation and offloading the animations:

1.) The animations need to be configured to useNativeDriver. This piece is quick and simple.
2.) onLayout needs to be moved to an implementation that leverages Animated.event.

Am I missing any other pieces here?

@nickhudkins
Copy link
Contributor

For those who want to follow along: #10289 implements this change

@narychen
Copy link
Contributor Author

narychen commented Oct 8, 2016

I've tried the current implementation of offloading. Found that a lot of property such as view's height cannot be animated in native. Will this be fixed in the future or just never be done?

@K-Leon
Copy link
Contributor

K-Leon commented Oct 14, 2016

@nickhudkins I've seen u closed your Pull Request. Native Events haven't been working well enough yet?

@nickhudkins
Copy link
Contributor

I ended up merging in master and butchering another merge and couldn't get
my test suite running properly so I closed it for now so that it isn't just
sitting open untested and messy. I'll re-open again once I can ensure
passing tests. Native Events however work wonderfully and make an enormous
improvement.
On Fri, Oct 14, 2016 at 7:51 AM Leon notifications@github.com wrote:

@nickhudkins https://github.com/nickhudkins I've seen u closed your
Pull Request. Native Events haven't been working well enough yet?


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#9421 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AASQditIjp-W0uOwKfxR6HwsyunF8a0sks5qz2y7gaJpZM4Jk_Gg
.

@K-Leon
Copy link
Contributor

K-Leon commented Oct 14, 2016

Great work! Thank you very much for your effort - I'm looking forward to see this landing it will be a real gamechanger.

@lacker
Copy link
Contributor

lacker commented Feb 10, 2017

The original question is answered so I am going to close this issue. Thanks folks for helping out!

@lacker lacker closed this as completed Feb 10, 2017
@facebook facebook locked as resolved and limited conversation to collaborators Jul 19, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jul 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Ran Commands One of our bots successfully processed a command. Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests

8 participants