-
Notifications
You must be signed in to change notification settings - Fork 199
Conversation
ah damn! wrong branching! |
@@ -481,10 +496,15 @@ class ExNavigationStack extends PureComponent<any, Props, State> { | |||
latestRouteConfig.navigationBar.visible !== false; | |||
|
|||
// TODO: add height and statusBarHeight options here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the todo
BTW, |
good point @satya164 . I updated the PR. |
let height = NavigationBar.DEFAULT_HEIGHT; | ||
|
||
if (latestRouteConfig.statusBar && latestRouteConfig.statusBar.translucent) { | ||
height = NavigationBar.DEFAULT_HEIGHT_WITHOUT_STATUS_BAR + 24; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Android, statusbar height is 25
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah. @brentvatne defined it at 24 already when in exponent env. I just mirrored that.
also for some reason 25 had a weird spacing issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chirag04 Hmm... okay, it's wrong though I remember when @brentvatne changed my app to exponent he had used 24 and it felt like the header title wasn't vertically centered (since it was 1px off).
What's the issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@satya164 updated the PR. I can't reproduce the 1px off thing in my app anymore. 25 works in my app for now.
// pass the statusBarHeight to headerComponent if statusBar is translucent | ||
let statusBarHeight = STATUSBAR_HEIGHT; | ||
if (latestRouteConfig.statusBar && latestRouteConfig.statusBar.translucent) { | ||
statusBarHeight = 24; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably keep this in a constant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
09acfc8
to
ab211a8
Compare
@brentvatne i'm unable to flow check this PR. for whatever reason it is checking react-native in node_modules. |
LGTM! :-D |
@chirag04 want to fix the merge conflicts here? |
3bfd7d2
to
4c52e4a
Compare
@skevy @brentvatne rebased. |
LGTM! |
@brentvatne This should work now: Closes #104 fbshipit-source-id: 5dd1eb8
@brentvatne
This should work now: