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

Use native bridge for animations #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dannycochran
Copy link
Owner

Motivation:
Rendering inconsistencies / slowness between screens in Android:
aksonov/react-native-router-flux#199
aksonov/react-native-router-flux#1177
aksonov/react-native-router-flux#1266

Fix
Basically patches in the change from https://github.com/facebook/react-native/pull/10289/files.

Once the above commit is merged into RN and pushed to a stable release, this can be merged. It won't be backwards compatible with older versions of RN, however.

I don't know how critical this fork is to the overall react-native-router-flux implementation, but long term it would probably be good to move off this fork altogether, or at least try to keep it up to date with the actual implementation of ExperimentalNavigation.

@aksonov would appreciate guidance here if you have thoughts?

Basically patches in the change from https://github.com/facebook/react-native/pull/10289/files.

Once the above commit is merged into RN and pushed to a stable release, this can be merged. It won't be backwards compatible with older versions of RN, however.
@aksonov
Copy link

aksonov commented Nov 15, 2016

Looks very good idea, unfortunately i can't test it, so maybe it is better to ask community to check?

@Kaishley
Copy link

@dannycochran Have the patches been merged yet in react-native?

@dannycochran
Copy link
Owner Author

@Kaishley no not yet, unfortunately. not sure why it's been in limbo so long.

@rossfishkind
Copy link

@dannycochran looks like the pull request has been closed.

@dannycochran
Copy link
Owner Author

I'll double check everything still works -- just FYI I've moved off of react-native-router-flux in favor of just using React Native Experimental Navigation. This forked dependency is significantly out of date and I'm not really sure what parts of it are modified compared to the actual implementation.

@dannycochran
Copy link
Owner Author

I just tested and this still works on RN 0.39.2.

@Kaishley @rossfishkind would you mind patching this change locally in your node_modules and confirming you see faster transitions while js debugging is enabled?

@K-Leon
Copy link

K-Leon commented Dec 16, 2016

@dannycochran to what solution did u migrate? Was it an easy switch?

@dannycochran
Copy link
Owner Author

@K-Leon it's just a few changes:

https://github.com/dannycochran/react-native-experimental-navigation/pull/1/files

@PulsarBlow
Copy link

Changing the NavigationAnimatedView.js with the code provided in this PR fixed slowness problem on my side.
🥇

@deathemperor
Copy link

Just applied and tested this PR for react-native-router-flux 3.38 with RN 0.43.3 and the animation slowness while enabling remote JS debugging is totally gone..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants