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

[NavigationExperimental] Kill NavigationAnimatedView #8269

Closed
wants to merge 1 commit into from

Conversation

jmurzy
Copy link
Contributor

@jmurzy jmurzy commented Jun 21, 2016

NavigationAnimatedView is deprecated. Use NavigationTransitioner.

As discussed in #8262, #8189.

NavigationAnimatedView is deprecated. Use NavigationTransitioner

Related to facebook#8262, facebook#8189
@ghost
Copy link

ghost commented Jun 21, 2016

By analyzing the blame information on this pull request, we identified @hedgerwang and @ericvicenti to be potential reviewers.

@ghost ghost added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jun 21, 2016
@jmurzy jmurzy changed the title Kill NavigationAnimatedView [NavigationExperimental ]Kill NavigationAnimatedView Jun 21, 2016
@jmurzy jmurzy changed the title [NavigationExperimental ]Kill NavigationAnimatedView [NavigationExperimental] Kill NavigationAnimatedView Jun 21, 2016
@ide
Copy link
Contributor

ide commented Jun 21, 2016

@ericvicenti can you take care of this? I'm thinking there might be callers of NavigationAnimatedView still within FB-internal code and deleting the file will break internal apps/tests.

@ericvicenti
Copy link
Contributor

@ide, you read my mind!

@ericvicenti
Copy link
Contributor

@facebook-github-bot import

@ghost
Copy link

ghost commented Jun 21, 2016

Thanks for importing. If you are an FB employee go to Phabricator to review.

@jmurzy
Copy link
Contributor Author

jmurzy commented Jun 21, 2016

@ericvicenti Also, there is a bug in NavigationTransitioner that needs to be fixed before we completely get rid of NavigationAnimatedView.

/cc @hedgerwang

🍺

@tpisto
Copy link

tpisto commented Jul 22, 2016

NavigationTransitioner is very very slow compared to previous NavigationAnimatedView. With NavigationAnimatedView the transition was instant, but now with NavigationTransitioner the same change takes like 3 seconds.

@ericvicenti
Copy link
Contributor

@tpisto, I don't think NavigationTransitioner is inherently much slower, but it depends on how it is used. Could you post a code snippet that demonstrates how slow it is now?

@tpisto
Copy link

tpisto commented Jul 23, 2016

Hi! Thank you for quick reply. I am using this solution:

https://github.com/jlyman/RN-NavigationExperimental-Redux-Example/blob/0.27/app/containers/AppContainer.js

But before I used AnimatedView. And with AnimatedView animations go like this:

screen recording 2016-07-23 at 01 31 pm

After updating to 0.30.0 and switching to Transitioner the animations go like this (not that I click the arrow down when the cursor moves slightly - so making many seconds the transition to happen):

screen recording 2016-07-23 at 12 51 pm

There are no any additional changes to code - just 0.29.2 -> 0.30.0 and AnimatedView -> Transitioner. (And Transitioner seems to need render instead of renderScene)

@arronmabrey
Copy link

@tpisto Is this still an issue you're facing or were you able to figure it out? Debating on upgrading to 0.30.0 but might hold off if you're still seeing this issue.

bubblesunyum pushed a commit to iodine/react-native that referenced this pull request Aug 23, 2016
Summary:
NavigationAnimatedView is deprecated. Use NavigationTransitioner.

As discussed in facebook#8262, facebook#8189.
Closes facebook#8269

Reviewed By: hedgerwang

Differential Revision: D3462087

Pulled By: ericvicenti

fbshipit-source-id: 6d7504cb8533f12a3610d34ef5b35edcf79fd5b7
mpretty-cyro pushed a commit to HomePass/react-native that referenced this pull request Aug 25, 2016
Summary:
NavigationAnimatedView is deprecated. Use NavigationTransitioner.

As discussed in facebook#8262, facebook#8189.
Closes facebook#8269

Reviewed By: hedgerwang

Differential Revision: D3462087

Pulled By: ericvicenti

fbshipit-source-id: 6d7504cb8533f12a3610d34ef5b35edcf79fd5b7
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants