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

Smoothly transition header visibility in Stack #3821

Merged
merged 2 commits into from Mar 25, 2018

Conversation

Projects
None yet
9 participants
@skevy
Member

skevy commented Mar 23, 2018

Summary

This addresses #2732 (which also happens to be the top issue on canny.io).

Transitioning between screens with and without headers when headerMode is set to "screen" works just fine, but the current experience when headerMode is set to "float" is very broken. The goal of this PR is to replicate the behavior of UIKit on iOS: when a screen without a header is pushed, the header on the previous screen is pushed off the screen to the left, and the individual components of the header do not animate. When a screen with a header is pushed after a screen without one, we slide the new header on the screen from right to left, in sync with the push animation, again while avoiding animating the individual header components.

I also updated the UIKit transition example in Navigation Playground to demonstrate this.

Here's a gif of the animation from a native iOS app:

gif of native ios transition

Test Plan:

Try the UIKit transition example in Navigation Playground.

@skevy skevy requested review from brentvatne and ericvicenti Mar 23, 2018

@skevy skevy force-pushed the @skevy/master.smoothly-transition-header branch 2 times, most recently from f600e44 to 0e6bafe Mar 23, 2018

Smoothly transition header visibility in Stack
This addresses #2732 (which also happens to be the top issue on canny.io).

Transitioning between screens with and without headers when `headerMode` is set to "screen" works just fine, but the current experience when `headerMode` is set to "float" is very broken. The goal of this PR is to replicate the behavior of UIKit on iOS: when a screen without a header is pushed, the header on the previous screen is pushed off the screen to the left, and the individual components of the header do not animate. When a screen with a header is pushed _after_ a screen without one, we slide the new header on the screen from right to left, in sync with the push animation, again while avoiding animating the individual header components.

I also updated the UIKit transition example in Navigation Playground to demonstrate this.

Test Plan:

Try the UIKit transition example in Navigation Playground.

@skevy skevy force-pushed the @skevy/master.update-react-native-scripts-in branch from 2264eb3 to 4490a41 Mar 23, 2018

@skevy skevy force-pushed the @skevy/master.smoothly-transition-header branch from 0e6bafe to 753f8e4 Mar 23, 2018

@react-navigation-ci

This comment has been minimized.

@codecov-io

This comment has been minimized.

codecov-io commented Mar 23, 2018

Codecov Report

Merging #3821 into @skevy/master.update-react-native-scripts-in will increase coverage by 0.08%.
The diff coverage is 76.92%.

Impacted file tree graph

@@                               Coverage Diff                                @@
##           @skevy/master.update-react-native-scripts-in    #3821      +/-   ##
================================================================================
+ Coverage                                         70.27%   70.36%   +0.08%     
================================================================================
  Files                                                53       53              
  Lines                                              1662     1694      +32     
================================================================================
+ Hits                                               1168     1192      +24     
- Misses                                              494      502       +8
Impacted Files Coverage Δ
src/views/Header/Header.js 67.58% <40%> (-1.21%) ⬇️
src/views/StackView/StackViewLayout.js 40.87% <75%> (+2.29%) ⬆️
src/views/Header/HeaderStyleInterpolator.js 61.66% <86.36%> (+11.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4490a41...fc795e4. Read the comment docs.

@brentvatne

this implementation is way cleaner than I expected it to be, nice work! just one small comment but otherwise lgtm

<View style={styles.flexOne}>{appBar}</View>
</SafeAreaView>
<Animated.View
style={[{ ...this.props.layoutInterpolator(this.props) }]}

This comment has been minimized.

@brentvatne

brentvatne Mar 23, 2018

Member

does this need to be an array? seems like style={{ ... this.props... etc }} would be sufficient

@ericvicenti

Thanks @skevy for addressing our top problem on canny!

first,
first + 0.001,
index - 0.9,
index - 0.2,

This comment has been minimized.

@ericvicenti

ericvicenti Mar 25, 2018

Contributor

These numbers scare me. Not sure what the alternative is, but it would be nice if the cross fading felt less hardcoded.

Of course these numbers were already here, so this change seems reasonable

This comment has been minimized.

@skevy

skevy Mar 26, 2018

Member

yah -- i didn't mess with that too hard. The thing that I added were the extra control points of first + .001 and last - .001 to the interpolation

outputRange: [
rtlMult * (hasHeader(scenes[first]) ? 0 : width),
rtlMult * (hasHeader(scenes[index]) ? 0 : isBack ? width : -width),
rtlMult * (hasHeader(scenes[last]) ? 0 : -width),

This comment has been minimized.

@ericvicenti

ericvicenti Mar 25, 2018

Contributor

Was this tested with RTL? This constraint is can be easy to forget about.. maybe we should add some info to the contributors guide to help people test edge cases.

This comment has been minimized.

@skevy

skevy Mar 26, 2018

Member

I did test with RTL, though unfortunately I think the swipe to go back gesture is broken in RTL mode.

@@ -433,11 +445,22 @@ class StackViewLayout extends React.Component {
screenInterpolator &&
screenInterpolator({ ...this.props.transitionProps, scene });
// If this screen has "header" set to `null` in it's navigation options, but
// it exists in a stack with headerMode float, add a negative margin to
// compensate for the hidden header

This comment has been minimized.

@ericvicenti

ericvicenti Mar 25, 2018

Contributor

hmm, so the whole card is moved up, interesting. This is probably the most reasonable thing for now, but I wonder if we should instead make the header hight padding some explicit thing in the future, and in this case it would just be 0.

This comment has been minimized.

@skevy

skevy Mar 26, 2018

Member

yah, that's probably fair. Maybe this can be addressed in future changes to Stack.

This comment has been minimized.

@janicduplessis

janicduplessis Apr 7, 2018

Contributor

I'm seeing some flickers sometimes on app load where a screen without header is offset by the header height, not 100% sure but it might be caused by this because we measure the header view.

It seems to only get fixed when the screen re-renders, so maybe using state here for headerLayout would be better, or have a way to use the same header padding value like @ericvicenti mentionned.

This comment has been minimized.

@janicduplessis

janicduplessis Apr 7, 2018

Contributor

I can confirm this is the issue, using state does help a bit but does not solve the issue. The only solution is to have the header height synchronously. It is exposed as a constant on the Header component but this will break custom header components if we use that.

@react-navigation-ci

This comment has been minimized.

@ericvicenti ericvicenti merged commit 3d8dd4f into @skevy/master.update-react-native-scripts-in Mar 25, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@cbjs

This comment has been minimized.

cbjs commented Apr 3, 2018

can this update solve the problem.
#3889

I tries newest 1.5.9, the problem still exists.

@brentvatne

This comment has been minimized.

Member

brentvatne commented Apr 3, 2018

yes but this only exists in the 2.0 branch

@wvteijlingen

This comment has been minimized.

wvteijlingen commented Apr 6, 2018

Thanks, this is a great improvement! Quick question though: Is it possible to have these smooth transitions when using a custom header component? I'm currently using 1.x with a custom header, but it seems that this you don't get this smooth transition when using a custom header in 2.x.

Perhaps it would be nice to separate the rendering of the header into two components. An outer container view that manages the visibility transitions, and the real header that is rendered into that container. That makes is easy to render a custom header component, and still have smooth visibility transitions.

Additionally the container component could then also extract the data that makes up the header, so we can simplify the API of the real header to a few easy to use props such as:
title, backButtonTitle, leftComponent, rightComponent, onBackPress.

@ericvicenti

This comment has been minimized.

Contributor

ericvicenti commented Apr 6, 2018

@janicduplessis

This comment has been minimized.

Contributor

janicduplessis commented Apr 7, 2018

@skevy @ericvicenti Awesome work on this! However there seems to be an issue when navigating between 2 screens with header: null. Let me know if you need a repro case.

animated gif-downsized_large

@wvteijlingen

This comment has been minimized.

wvteijlingen commented Apr 7, 2018

@ericvicenti Too bad it doesn't work with custom headers, but I understand given the current header layout.

Would it be possible to separate the rendering of the header into two components? An outer container view that manages the visibility transitions, and the real header that is rendered into that container. That makes is easy to render a custom header component, and still have smooth visibility transitions.

@brentvatne brentvatne deleted the @skevy/master.smoothly-transition-header branch May 7, 2018

@ragunaru

This comment has been minimized.

ragunaru commented Jun 10, 2018

I'm on 2.2.0 and the header transition bug is still not fixed?

@brentvatne

This comment has been minimized.

Member

brentvatne commented Jun 10, 2018

@ragunaru - it sounds like you may be seeing a different issue because this is indeed working as expected. please create a new issue and follow the template and i'll be happy to look into it!

@react-navigation react-navigation locked and limited conversation to collaborators Jun 10, 2018

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