Skip to content

Commit

Permalink
Pass the current and previous transition props to render, `onTransi…
Browse files Browse the repository at this point in the history 
…tionStart` and `onTransitionEnd`.

Summary: This shall make it convenient to handle transition changes.

Reviewed By: ericvicenti

Differential Revision: D3442291

fbshipit-source-id: aee0ffe18ada40ef133484b4a4999f282c66c181
  • Loading branch information
Hedger Wang authored and Facebook Github Bot 4 committed Jun 21, 2016
1 parent 6982f5a commit c57bac4
Show file tree
Hide file tree
Showing 2 changed files with 73 additions and 39 deletions.
97 changes: 67 additions & 30 deletions Libraries/NavigationExperimental/NavigationTransitioner.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ const React = require('React');
const StyleSheet = require('StyleSheet');
const View = require('View');

const invariant = require('fbjs/lib/invariant');

import type {
NavigationAnimatedValue,
NavigationLayout,
Expand All @@ -33,7 +35,7 @@ type Props = {
navigationState: NavigationState,
onTransitionEnd: () => void,
onTransitionStart: () => void,
render: (props: NavigationTransitionProps) => any,
render: (a: NavigationTransitionProps, b: ?NavigationTransitionProps) => any,
style: any,
};

Expand All @@ -56,9 +58,10 @@ function isSceneNotStale(scene: NavigationScene): boolean {
}

class NavigationTransitioner extends React.Component<any, Props, State> {

_onLayout: (event: any) => void;
_onTransitionEnd: () => void;
_prevTransitionProps: ?NavigationTransitionProps;
_transitionProps: NavigationTransitionProps;

props: Props;
state: State;
Expand Down Expand Up @@ -90,6 +93,9 @@ class NavigationTransitioner extends React.Component<any, Props, State> {
progress: new Animated.Value(1),
scenes: NavigationScenesReducer([], this.props.navigationState),
};

this._prevTransitionProps = null;
this._transitionProps = buildTransitionProps(props, this.state);
}

componentWillMount(): void {
Expand All @@ -108,15 +114,21 @@ class NavigationTransitioner extends React.Component<any, Props, State> {
return;
}

const nextState = {
...this.state,
scenes: nextScenes,
};

this._prevTransitionProps = this._transitionProps;
this._transitionProps = buildTransitionProps(nextProps, nextState);

const {
position,
progress,
} = this.state;
} = nextState;

// update scenes.
this.setState({
scenes: nextScenes,
});
this.setState(nextState);

// get the transition spec.
const transitionUserSpec = nextProps.configureTransition ?
Expand Down Expand Up @@ -153,7 +165,10 @@ class NavigationTransitioner extends React.Component<any, Props, State> {
}

// play the transition.
nextProps.onTransitionStart && nextProps.onTransitionStart();
nextProps.onTransitionStart && nextProps.onTransitionStart(
this._transitionProps,
this._prevTransitionProps,
);
Animated.parallel(animations).start(this._onTransitionEnd);
}

Expand All @@ -162,7 +177,7 @@ class NavigationTransitioner extends React.Component<any, Props, State> {
<View
onLayout={this._onLayout}
style={[styles.main, this.props.style]}>
{this.props.render(this._buildTransitionProps())}
{this.props.render(this._transitionProps, this._prevTransitionProps)}
</View>
);
}
Expand All @@ -184,33 +199,55 @@ class NavigationTransitioner extends React.Component<any, Props, State> {
}

_onTransitionEnd(): void {
const prevTransitionProps = this._prevTransitionProps;
this._prevTransitionProps = null;

const scenes = this.state.scenes.filter(isSceneNotStale);
if (scenes.length !== this.state.scenes.length) {
this.setState({ scenes });
}
this.props.onTransitionEnd && this.props.onTransitionEnd();
}
this.setState({ scenes });

_buildTransitionProps(): NavigationTransitionProps {
const {
navigationState,
} = this.props;
this.props.onTransitionEnd && this.props.onTransitionEnd(
this._transitionProps,
prevTransitionProps,
);
}
}

const {
layout,
position,
progress,
scenes,
} = this.state;
function buildTransitionProps(
props: Props,
state: State,
): NavigationTransitionProps {
const {
navigationState,
} = props;

const {
layout,
position,
progress,
scenes,
} = state;

return {
layout,
navigationState,
position,
progress,
scenes,
scene: findActiveScene(scenes, navigationState.index),
};
}

return {
layout,
navigationState,
position,
progress,
scenes,
};
function findActiveScene(
scenes: Array<NavigationScene>,
index: number,
): NavigationScene {
for (let ii = 0, jj = scenes.length; ii < jj; ii++) {
const scene = scenes[ii];
if (!scene.isStale && scene.index === index) {
return scene;
}
}
invariant(false, 'scenes must have an active scene');
}

const styles = StyleSheet.create({
Expand Down
15 changes: 6 additions & 9 deletions Libraries/NavigationExperimental/NavigationTypeDefinition.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,19 +64,16 @@ export type NavigationTransitionProps = {

// All the scenes of the transitioner.
scenes: Array<NavigationScene>,
};

export type NavigationSceneRendererProps = {
layout: NavigationLayout,
navigationState: NavigationState,
position: NavigationAnimatedValue,
progress: NavigationAnimatedValue,
scenes: Array<NavigationScene>,

// The scene to render.
// The active scene, corresponding to the route at
// `navigationState.routes[navigationState.index]`.
scene: NavigationScene,
};

// Similar to `NavigationTransitionProps`, except that the prop `scene`
// represents the scene for the renderer to render.
export type NavigationSceneRendererProps = NavigationTransitionProps;

export type NavigationPanPanHandlers = {
onMoveShouldSetResponder: Function,
onMoveShouldSetResponderCapture: Function,
Expand Down

10 comments on commit c57bac4

@dozoisch
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hedgerwang I am under the impression that this change prevents the old view from unmounting.

When I use this version, compared to the one that is in 0.29-rc.1 right now I see a shadow artifact on the right of the view when pressing back. Also if I add log in componentWillUnmount the previous view, only unmounts when another transition occurs.

@hedgerwang
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dozoisch : stale scenes should have been removed when the transition finishes.

react-native/Libraries/NavigationExperimental/NavigationTransitioner.js

Line 206 in c57bac4

this.setState({ scenes });

please let me know if this is not happening for you. Thanks.

@ryankask
Copy link
Contributor

@ryankask ryankask commented on c57bac4 Jun 22, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dozoisch I'm not sure if this is related/what you mean but I've always noticed an issue with NavigationCard having its drop shadow "linger" before unmounting after pressing back. It's present in the below screenshot (it seems sped up).

I've had a look at some other iOS apps and it seems like shadowOpacity should animate to 0.

1-xuqhfizckl6xtaf2-zao3a

source: www.reactnative.com

@dozoisch
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is using the version in master:

image

This is using the version in 0.29:
image

@hedgerwang so in master, you can see the following flow:

  • Go to Rounds
  • Pressed back (view still mounted)

--- This is needed for unmounting

  • Pressed go to chats
  • unmounts rounds

in 0.29 which is the correct behavior:

  • Go to Rounds
  • Pressed back
  • Unmounts rounds

--- (this is not needed for unmounting)

  • Press go to chats

So it does seem like the views doesn't get unmounted properly. And with master version I get the shadow lingering, where as with the 0.29 version of transitionner the shadow disappears.

@dozoisch
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ryankask Good question! Well in 0.29 I see the shadow disappearing exactly like in your gif, with the transitionner from masters, it stays there forever

@hedgerwang
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dozoisch : could you plz patch this to NavigationTransitioner and see if this fixes your issue?

_onTransitionEnd(): void {
    const prevTransitionProps = this._prevTransitionProps;
    this._prevTransitionProps = null;

    const nextState = {
      ...this.state,
      scenes: this.state.scenes.filter(isSceneNotStale),
    };

    this._transitionProps = buildTransitionProps(this.props, nextState);

    this.setState(nextState);

    this.props.onTransitionEnd && this.props.onTransitionEnd(
      this._transitionProps,
      prevTransitionProps,
    );
  }
}

cc @ericvicenti

@dozoisch
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hedgerwang Alright, so I repasted the transitionner from master. Bug still hapenning (as expected). Applied your fix:

It works now! It unmounts properly 🎉

image

@hedgerwang
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dozoisch : The fix will be land soon. Thanks for helping out to test this 👍

@ericvicenti
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I bumped into the same issue this morning- the fix is landing now.

Thanks for braving our master branch and catching this @dozoisch!

@dozoisch
Copy link
Contributor

@dozoisch dozoisch commented on c57bac4 Jun 22, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ericvicenti @hedgerwang Thanks for being so responsive :)!

Yeah I've been following the changes to Navigation really closely, looking forward to see the "Experimental" being removed!

Please sign in to comment.