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

AnimatedView incorrectly renders focused scene when index !== children.length-1 #61

Closed
jmurzy opened this issue Apr 9, 2016 · 5 comments

Comments

@jmurzy
Copy link

jmurzy commented Apr 9, 2016

In the NavigationCompositionExample, tabs are implemented using a NavigationView. NavigationView renders a single Scene and does so by unmounting any previous Scenes. This is not desirable as component state is lost when switching tabs.

I'm trying to implement tabs using AnimatedView and StackReducer to keep the views mounted when navigated to and from using JumpToIndex or JumpTo.

      +------------+
    +-+            |
  +-+ |            |
  | | |            |
  | | |  Focused   |
  | | |   Card     |
  | | |            |
  +-+ |            |
    +-+            |
      +------------+

AnimatedView renders each Card in the order Scenes are present in ParentState. It assumes the children.length-1 is the focused Card. So setting index in ParentState to anything other than the last Scene has no effect except that the overlay renders correctly since it's passed the Scene at the correct index when render is called:

renderOverlay({
        layout: this._layout,
        navigationState,
        onNavigate,
        position,
        scene: scenes[navigationState.index],
        scenes,
      });

As for the Scenes, the focused Card renders as the one at children.length-1. So the index provided by the reducer is ignored.

One way to fix this is to look at how AnimatedView delegates transition management to StyleInterpolators.

Take forHorizontal for example:

function forHorizontal(props: NavigationSceneRendererProps): Object {
  const {
    layout,
    position,
    scene,
  } = props;

  const index = scene.index;
  const inputRange = [index - 1, index, index + 1];
  const width = layout.initWidth;

  const opacity = position.interpolate({
    inputRange,
    outputRange: [1, 1, 0.3],
  });

  const scale = position.interpolate({
    inputRange,
    outputRange: [1, 1, 0.95],
  });

  const translateY = 0;
  const translateX = position.interpolate({
    inputRange,
    outputRange: [width, 0, -10],
  });

  return {
    opacity,
    transform: [
      { scale },
      { translateX },
      { translateY },
    ],
  };
}

Here the implementation "configures" the focused Card to be the last in the stack. Since, StyleInterpolator is the delegate that coordinates how the actual views transition on the screen, unlike AnimatedView, it's possible to change the implementation to render the focused view correctly and make transition animations work as expected.

The solution 🚧 I came up with is to push the views to the right of the focused Card off screen:

function forHorizontal(props: NavigationSceneRendererProps): Object {
  const {
    layout,
    position,
    scene,
  } = props;

  const index = scene.index;
  const inputRange = [index - 1, index, index + 1];
  const width = layout.initWidth;
  const translateY = 0;

  if(props.navigationState.index < scene.index && width <= 0) {
   // Move off screen
    const translateX = layout.width.interpolate({
      inputRange: [0, 1],
      outputRange: [0, -1],
    });

    return {
      opacity: 0, // Avoid flicker
      transform: [
        { translateX },
        { translateY },
      ],
    };
  }

  const opacity = position.interpolate({
    inputRange,
    outputRange: [1, 1, 0.3],
  });

  const scale = position.interpolate({
    inputRange,
    outputRange: [1, 1, 0.95],
  });

  const translateX = position.interpolate({
    inputRange,
    outputRange: [width, 0, -10],
  });

  return {
    opacity,
    transform: [
      { scale },
      { translateX },
      { translateY },
    ],
  };
}

Here I'm using width <= 0 😭 to determine the direction of navigation. A better way would be to include a prevIndex value in the ParentState type so that we can do this:

navigationState.index >= navigationState.prevIndex

Of course, we'd also have to make some changes to jumpToIndex, jumpTo etc in NavigationStateUtils to implicitly keep track of prevIndex.

Alternatively, we could create a custom reducer to achieve pretty much the same thing. But that feels like boilerplate.

{
  ...NavigationStateUtils.jumpToIndex(state, state.index+1),
  prevIndex: state.index
}

For those of you who are looking into implementing tabs using this approach, the transition animations can be "turned off" for tabs by supplying an applyAnimation fn to AnimatedView:

function applyAnimation(
position: NavigationAnimatedValue,  
navigationState: NavigationParentState): void {
  position.setValue(navigationState.index);
}

<NavigationAnimatedView
  applyAnimation={applyAnimation}
  ... />

So far this solution has been working great. But I strongly think some of this can be moved into NavigationExperimental.

I'd be happy to turn some of these ideas into a PR. Or perhaps there is another way to achieve this that I'm missing?

Oh and, thanks for all the work you guys have been doing on these. 🚀

🍺

@hedgerwang
Copy link
Collaborator

I'm working on the fix now. sorry about the inconvenience.

@hedgerwang
Copy link
Collaborator

The root cause of this is that the initialWidth is always 0 when the component is mounted. The non-zero value will be set at NavigationAnimatedView#_onLayout, which should re-render the component.

@jmurzy
Copy link
Author

jmurzy commented Apr 11, 2016

I'm not sure if that is the root cause but that's why I'm interpolating the width value in forHorizontal. As soon as a width is available, translateX moves it off the screen by as much, and opacityis set to 0 so we can avoid the flicker.

// Move off screen
const translateX = layout.width.interpolate({
  inputRange: [0, 1],
  outputRange: [0, -1],
});

ghost pushed a commit to facebook/react-native that referenced this issue Apr 19, 2016
Summary:The initial layout used to render scenes does not contain the actual
width and height measured and causes the issue as described at
ericvicenti/navigation-rfc#61

The fix is to update the layout and re-render scenes once layout
is modified. Also scenes renderer should also consider the case that
when the layout is not measured yet.

Reviewed By: ericvicenti

Differential Revision: D3162143

fb-gh-sync-id: 197574329d3849cad2a21e07e1bd5e800f74c3ea
fbshipit-source-id: 197574329d3849cad2a21e07e1bd5e800f74c3ea
@hedgerwang
Copy link
Collaborator

@jmurzy : we've added a new property isMeasure to the layout so that the interpolator may use it to handle the initial rendering.

Also, we've fixed the issue that the initial scenes aren't positioned properly.

facebook/react-native@81c62c5

@jmurzy
Copy link
Author

jmurzy commented Apr 21, 2016

@hedgerwang Fixed in ^ that commit. Thanks!

🍻

@jmurzy jmurzy closed this as completed Apr 21, 2016
ghost pushed a commit to facebook/react-native that referenced this issue Apr 25, 2016
Summary:
The initial layout used to render scenes does not contain the actual
width and height measured and causes the issue as described at
ericvicenti/navigation-rfc#61

The fix is to update the layout and re-render scenes once layout
is modified. Also scenes renderer should also consider the case that
when the layout is not measured yet.

Reviewed By: ericvicenti

Differential Revision: D3203245

fb-gh-sync-id: 4de89b9b43bc993d7c970c831458bd31c094073e
fbshipit-source-id: 4de89b9b43bc993d7c970c831458bd31c094073e
ptmt pushed a commit to ptmt/react-native that referenced this issue May 9, 2016
Summary:The initial layout used to render scenes does not contain the actual
width and height measured and causes the issue as described at
ericvicenti/navigation-rfc#61

The fix is to update the layout and re-render scenes once layout
is modified. Also scenes renderer should also consider the case that
when the layout is not measured yet.

Reviewed By: ericvicenti

Differential Revision: D3162143

fb-gh-sync-id: 197574329d3849cad2a21e07e1bd5e800f74c3ea
fbshipit-source-id: 197574329d3849cad2a21e07e1bd5e800f74c3ea
ptmt pushed a commit to ptmt/react-native that referenced this issue May 9, 2016
Summary:
The initial layout used to render scenes does not contain the actual
width and height measured and causes the issue as described at
ericvicenti/navigation-rfc#61

The fix is to update the layout and re-render scenes once layout
is modified. Also scenes renderer should also consider the case that
when the layout is not measured yet.

Reviewed By: ericvicenti

Differential Revision: D3203245

fb-gh-sync-id: 4de89b9b43bc993d7c970c831458bd31c094073e
fbshipit-source-id: 4de89b9b43bc993d7c970c831458bd31c094073e
zebulgar pushed a commit to nightingale/react-native that referenced this issue Jun 18, 2016
Summary:The initial layout used to render scenes does not contain the actual
width and height measured and causes the issue as described at
ericvicenti/navigation-rfc#61

The fix is to update the layout and re-render scenes once layout
is modified. Also scenes renderer should also consider the case that
when the layout is not measured yet.

Reviewed By: ericvicenti

Differential Revision: D3162143

fb-gh-sync-id: 197574329d3849cad2a21e07e1bd5e800f74c3ea
fbshipit-source-id: 197574329d3849cad2a21e07e1bd5e800f74c3ea
zebulgar pushed a commit to nightingale/react-native that referenced this issue Jun 18, 2016
Summary:
The initial layout used to render scenes does not contain the actual
width and height measured and causes the issue as described at
ericvicenti/navigation-rfc#61

The fix is to update the layout and re-render scenes once layout
is modified. Also scenes renderer should also consider the case that
when the layout is not measured yet.

Reviewed By: ericvicenti

Differential Revision: D3203245

fb-gh-sync-id: 4de89b9b43bc993d7c970c831458bd31c094073e
fbshipit-source-id: 4de89b9b43bc993d7c970c831458bd31c094073e
samerce pushed a commit to iodine/react-native that referenced this issue Aug 23, 2016
Summary:The initial layout used to render scenes does not contain the actual
width and height measured and causes the issue as described at
ericvicenti/navigation-rfc#61

The fix is to update the layout and re-render scenes once layout
is modified. Also scenes renderer should also consider the case that
when the layout is not measured yet.

Reviewed By: ericvicenti

Differential Revision: D3162143

fb-gh-sync-id: 197574329d3849cad2a21e07e1bd5e800f74c3ea
fbshipit-source-id: 197574329d3849cad2a21e07e1bd5e800f74c3ea
samerce pushed a commit to iodine/react-native that referenced this issue Aug 23, 2016
Summary:
The initial layout used to render scenes does not contain the actual
width and height measured and causes the issue as described at
ericvicenti/navigation-rfc#61

The fix is to update the layout and re-render scenes once layout
is modified. Also scenes renderer should also consider the case that
when the layout is not measured yet.

Reviewed By: ericvicenti

Differential Revision: D3203245

fb-gh-sync-id: 4de89b9b43bc993d7c970c831458bd31c094073e
fbshipit-source-id: 4de89b9b43bc993d7c970c831458bd31c094073e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants