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

Use Header.HEIGHT instead of measuring to avoid flicker #3940

Merged
merged 2 commits into from Apr 9, 2018

Conversation

Projects
None yet
5 participants
@janicduplessis
Contributor

janicduplessis commented Apr 9, 2018

The current technique we use to measure the header doesn't work properly because it is async and will cause the screen to jump. The main problem is that there's no way to know the height of the header synchronously for custom headers. For the default one we know the height already so it is pretty easy (although someone could use headerStyles to change the height and would break this).

This is mostly just to discuss the best way to fix this, I don't think using Header.HEIGHT is fine since it breaks custom header height but can be a good temporary patch for now.

I can see 2 possible solutions:

  1. Try to read style props to figure out the height (might not be 100% reliable).
  2. Add a navigationOption to customize the header height.

Test plan (required)

Tested that this fixes the flicker in the app where I noticed it.

@janicduplessis

This comment has been minimized.

Contributor

janicduplessis commented Apr 9, 2018

@react-navigation-ci

This comment has been minimized.

@janicduplessis janicduplessis force-pushed the janicduplessis:header-flicker branch from 9bb2524 to d634390 Apr 9, 2018

@react-navigation-ci

This comment has been minimized.

1 similar comment
@react-navigation-ci

This comment has been minimized.

@codecov-io

This comment has been minimized.

codecov-io commented Apr 9, 2018

Codecov Report

Merging #3940 into master will increase coverage by 0.06%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3940      +/-   ##
==========================================
+ Coverage   68.45%   68.51%   +0.06%     
==========================================
  Files          57       57              
  Lines        1756     1753       -3     
==========================================
- Hits         1202     1201       -1     
+ Misses        554      552       -2
Impacted Files Coverage Δ
src/views/Header/Header.js 68.53% <ø> (+0.94%) ⬆️
src/views/StackView/StackViewLayout.js 40.44% <66.66%> (-0.44%) ⬇️

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 e5e2cbb...2faedfe. Read the comment docs.

@ericvicenti

This seems like a good decision, given the current implementation of the Header, in which most things are hardcoded.

Ultimately I’d hope for a header api which allows each header to lay out individually, perhaps resulting in different heights, yet still manage to transition seamlessly without flickering.

@react-navigation-ci

This comment has been minimized.

@ericvicenti ericvicenti merged commit 5274d16 into react-navigation:master Apr 9, 2018

1 check passed

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

This comment has been minimized.

Member

skevy commented Apr 11, 2018

@janicduplessis take a look at the NavigationPlayground on master, in the "UIKit-style header transitions" section, on iPhone X. With this PR, there's a black bar at the top of the screen. When I revert it, it looks correct. I think this is probably because Header.HEIGHT is not always the same, especially where the iPhone X is concerned. Any thoughts?

@janicduplessis

This comment has been minimized.

Contributor

janicduplessis commented Apr 11, 2018

@skevy Right, I forgot to test that. I guess iPhone X support is implemented using SafeAreaView. Not sure what’s the best way to go about this, the simpler solution I can think of is to make Header.HEIGHT return the proper height when on iphone X.

@janicduplessis

This comment has been minimized.

Contributor

janicduplessis commented Apr 11, 2018

I'm currently using:

const { width, height } = Dimensions.get('window');
const isPhoneX =
  Platform.OS === 'ios' &&
  !Platform.isPad &&
  !Platform.isTVOS &&
  (height === 812 || width === 812);

to detect iphoneX in JS... Far from ideal but it could work for now. We'll have to make sure to test screen rotation too.

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