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

Shared element transition #941

Closed
wants to merge 58 commits into
base: master
from

Conversation

Projects
None yet
@lintonye
Contributor

lintonye commented Apr 6, 2017

Motivation

See #175, this PR is the first part: built-in support for shared element transition. The second part would be an API for custom transitions, which would come after the implementation of shared element transition becomes solid.

Here are some videos: on Android (Nexus 5X, Nougat), on iOS simulator.

Usage Example

import { Transition } from 'react-navigation';
// PhotoGrid.js
<Transition.SharedElement.Image id='image1' ... />
// PhotoDetail.js
<Transition.SharedElement.Image id='image1' ... />

If the "from" and "to" routes contain elements with matching ids, shared element transition will be played. The actual animation is a combination of cross fade and shared element transition.

Otherwise, the platform default transition (as implemented in CardStyleInterpolator) will be used.

Similar to Animated, built-in components include Transition.SharedElement.Image, Transition.SharedElement.View and Transition.SharedElement.Text. Transition.SharedElement.createComponent() can be used to create a shared-element component from another component.

Quick guide to the code

This PR includes changes in the following areas:

  • CardStack is slightly modified to wrap the original CardStack component with a HOC withTransition. Code related to transitions is moved to withTransition.js.
  • Card is modified to render to <Transition.View> instead of <Animated.View>. A couple of context fields are added to propagate route and transition information to its descendants.
  • src/views/Transition: This is the "meat", which includes main classes and functions that add custom transition support.
  • src/views/__tests__: Added some unit tests
  • examples/NavigationPlayground: An example app is added here. All the images are from unsplash.com which should not introduce any license issues.

Test plan

  • Test on both platforms with and without Transition.SharedElement items
  • Test share element transition using different components, including Image, View, Text and custom components
  • Verify that the platform default transition is used when there are no shared elements with matching ids
  • Perform normal user interaction to the interface, such as scrolling, and verify that the shared element transition behaves correctly
  • Test the performance impact on the UI after adding shared elements

Known issues

  • Animating fontSize of <Text> is implemented but not working yet.
  • The frame rate on iOS seems to be lower than on Android (Nexus 5X Nougat).

lintonye added some commits Mar 31, 2017

Merge branch 'master' into multi-transitions-rewrite
* master:
  Docs: Added clarification for header configuration (#891)
  Fix gesturesEnabled regression (#886)
  bump react-native-drawer-layout-polyfill (#882)
  Fix rebase commands (#870)
  Possibility to overwrite label's style if defined as string. (#731)
  add example for DrawerNavigatorConfig (#552)
  Workaround for screenProps in TabViewAnimated (#862)
  Update Guide-Nested.md (#813)
  remove ReactComponentWithPureRenderMixin (#809)
Merge branch 'multi-transitions-rewrite' into shared-element
* multi-transitions-rewrite:
  initTransition => bindTransition
Merge branch 'multi-transitions-rewrite' into shared-element
* multi-transitions-rewrite:
  Add unit tests for transitions
  Update comments
  Use default transition if can't find matching one
  Default transition
Merge branch 'multi-transitions-rewrite' into shared-element
* multi-transitions-rewrite:
  Restructure a bit _findTransitionContainer
Merge branch 'master' into shared-element
* master:
  add jest config for react-navigation in docs (#256) (#331)
@ericvicenti

This is exciting but it feels like a bit more complexity than is needed. For example, do we really need 'compositions' to implement the shared element functionality? Also it seems there is more indirection than needed with extra classes like TransitionItems.

Are there additional features being added here, other than shared element? I hope we can simplify it. For example, I see that there is additional logic to coordinate the CrossFade, which may be nice but I think should be done separately from the shared element implementation. I think the existing CardStack transition should work with shared elements.

Also, a bit of mechanical feedback: lets avoid using lodash and instead use language features. Also, is flow passing? I see it is overlooked in a few places

// already registered.
const onLayout = async () => {
const toRouteName = that._toRoute && that._toRoute.routeName;

This comment has been minimized.

@ericvicenti

ericvicenti Apr 6, 2017

Contributor

why do you need that here? Shouldn't this work?

@@ -571,6 +523,8 @@ class CardStack extends Component<DefaultProps, Props, void> {
const SceneComponent = this.props.router.getComponentForRouteName(
props.scene.route.routeName
);
const extraProps = this.props.createExtraSceneProps &&

This comment has been minimized.

@ericvicenti

ericvicenti Apr 6, 2017

Contributor

I think we should avoid calling this callback from the render path. Instead I think it belongs in componentDidUpdate or componentWillUpdate. That way, we can clear old scenes that are gone

This comment has been minimized.

@lintonye

lintonye Apr 6, 2017

Contributor

This callback requires the NavigationSceneRendererProps of the current scene that is being rendered. The props is used to interpolate styles (which result in the animations). Since NavigationSceneRenderProps is only available in the render callback of the Transitioner, I'm not sure how to move that to componentDidUpdate or componentWillUpdate?

What are the downsides of using it in the render path? Performance issue?

lintonye added some commits Apr 6, 2017

@lintonye

This comment has been minimized.

Contributor

lintonye commented Apr 6, 2017

@browniefed @ericvicenti Thanks for the feedback. I'll fix those flow errors and ListView in a few days.

A few things that I'd appreciate further clarification:

For example, do we really need 'compositions' to implement the shared element functionality?

I see that there is additional logic to coordinate the CrossFade

Composition with CrossFade is needed because simply doing shared element transition and then immediately switching to the new scene doesn't look good IMHO. I'll upload a video for your comparison later.

This PR is based on my previous work on custom transitions. Yes it contains extra features, i.e. the support for custom transitions. This is the easiest way that I can see compared to trying to extract shared-element only code. After all, most of the code in this implementation is required for shared-element transition, such as creating an overlay, cloning items, and measuring view bounds. I just generalized them and made them usable by other transition types. If we have any plan to support custom transitions, trying to make the code specific to shared element seems counterproductive to me.

Also it seems there is more indirection than needed with extra classes like TransitionItems.

How so? Would renaming it to SharedElements or something address the indirection issue? Or did you mean something else?

I think the existing CardStack transition should work with shared elements.

Did you mean the CardStyleInterpolator etc? This PR actually uses those classes to create default platform transitions. It's more of a "container" of existing transition though since we need a lot more logic to implement shared element transition. Does this make sense?

lets avoid using lodash

I could replace lodash. I used it because it's convenient and it's in the dependency list of react-native: https://github.com/facebook/react-native/blob/master/package.json#L178. Will it eventually be removed there?

@ericvicenti

This comment has been minimized.

Contributor

ericvicenti commented Apr 10, 2017

So I think that a more advanced transition API would be nice, but we seem to have a long way to go before the API is decided on. If we try to implement it before the API is fully designed, we are likely to wind up with a very confusing implementation.

If we have any plan to support custom transitions, trying to make the code specific to shared element seems counterproductive to me.

I am concerned by adding all this extra code for generic custom transitions, when we aren't even sure what that API will look like. Instead, maybe this PR could focus on adding shared transitions, while leaving the card transition intact? So that the card will still come from the right or bottom, but the shared elements will float to the destination location.

Did you mean the CardStyleInterpolator etc? This PR actually uses those classes to create default platform transitions. It's more of a "container" of existing transition though since we need a lot more logic to implement shared element transition. Does this make sense?

I'm not sure exactly why the card style needs to change at all in this diff. We should just be adding shared elements.

lodash

Ah, the lodash usage seems OK. I didn't notice that RN already uses it. Generally it is best to use language features when possible, but this usage actually seems fine.

Also it seems there is more indirection than needed with extra classes like TransitionItems.

My main question here is: why do we need a full class for TransitionItems? Couldn't this data be represented with a bit of state in an existing module?

@lintonye

This comment has been minimized.

Contributor

lintonye commented Apr 19, 2017

To everyone who's watching this PR, as discussed with @ericvicenti, we've decided to do another round of refinement of the custom transitions API, before moving on with shared element transition. I'll post updates to the proposal on #175. Please stay tuned!

@tlvenn

This comment has been minimized.

tlvenn commented Apr 25, 2017

Thanks for the update and your hard work on this @lintonye ! Much appreciated.

@FacuAcosta

Users need more transitioning options and this PR is perfect for that

@Sam1301 Sam1301 referenced this pull request Jun 12, 2017

Merged

Add slide animation view. #682

@chrsk

This comment has been minimized.

chrsk commented Jul 12, 2017

Are you currently working on the open tasks? Do you need any help?

@nonameolsson

This comment has been minimized.

nonameolsson commented Aug 24, 2017

I would love to see this one merged! What's missing to get it merged?

@JCMais

This comment has been minimized.

JCMais commented Sep 5, 2017

Can we please get an update on the status of this PR?

@ronal2do

This comment has been minimized.

ronal2do commented Oct 14, 2017

Any update?

@nonameolsson

This comment has been minimized.

nonameolsson commented Oct 27, 2017

What is the state on this?

@behzad888

This comment has been minimized.

behzad888 commented Jan 31, 2018

What is the state on this? @lintonye

@brentvatne

This comment has been minimized.

Member

brentvatne commented Feb 9, 2018

this is an incredible effort! it seems like now it's a bit out of date -- @lintonye if you'd like to pick up where you left off here, @ericvicenti and I are working on react-navigation close to full-time now and we can support you in this with quick reviews and discussions. let's move over to react-navigation/rfcs#17 and turn this into a RFC, then circle back to a PR once that's ready.

sorry to everyone who is waiting for this, feel free to weigh in on the rfc thread mentioned above with your ideas around this API, or to vote on https://react-navigation.canny.io/feature-requests/p/shared-element-transitions if this is important to you.

@brentvatne brentvatne closed this Feb 9, 2018

@react-navigation react-navigation locked and limited conversation to collaborators Feb 9, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.