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

fix: fixes header calculation height #596

Merged
merged 8 commits into from
Oct 27, 2020
Merged

Conversation

mikaelbr
Copy link
Collaborator

Seem to be a regression when updating some dependencies I think, where the calculation of the header height in full screen mode is off. This "fixes" that.

The calculations are based on hard-coded values as specified in the react-navigation tab bar code. Not ideal and brittle for cases such as this, but the alternative is layout listener propagating through from the tab bar, which is not ideal either. Might be some other solutions as well. But for now just compensate for the changes and fix the annoying gap

image

(also included a minor performance improvements that I don't want to do as a separate PR)

@rikkekantega
Copy link
Contributor

rikkekantega commented Oct 23, 2020

Screenshot_20201023-163448_atb
This gap is still pretty large here. Might be using the wrong OS🤔

@mikaelbr
Copy link
Collaborator Author

What are you using (os and device)? I'll reproduce and look into it

@rikkekantega
Copy link
Contributor

What are you using (os and device)? I'll reproduce and look into it

Samsung galaxy s20 / Android 10

@cbrevik
Copy link
Contributor

cbrevik commented Oct 23, 2020

but the alternative is layout listener propagating through from the tab bar, which is not ideal either.

Might be that is the best alternative here? Put the TabBar-component in AppContextProvider or something, and provide the tabbar-height as a context value there as well? Bit eeeeh, but at the same time might be more robust across platforms?

@mikaelbr
Copy link
Collaborator Author

The issue seem to be status bar height on Android. I also changed the status bar background color on notched phones:

image

@mikaelbr
Copy link
Collaborator Author

This issue is odd as it seems to be different on solutions with notch and non-notch, but hard to verify as Emulators aren't treating notches properly (react-native-device-info/react-native-device-info#831). Also @rikkekantega doesn't have a notch on her phone, but still see the issue. But not able to reproduce on emulators.

@rikkekantega
Copy link
Contributor

rikkekantega commented Oct 26, 2020

Status bar color is changed on non-notched phones as well, I am assuming this is intended.:)

Before:
Screenshot_20201026-150958_atb

After:
Screenshot_20201026-150942_atb

@mikaelbr
Copy link
Collaborator Author

@rikkekantega This is apparently more complicated than I would've thought. There are issues with the emulators (running three of them). Could you check now?

@mikaelbr
Copy link
Collaborator Author

Ahh. Hah. 😅 Didn't see the comment. Thanks! But I think maybe the latest changes (2 min ago) aren't included there? Could you please check again?

@rikkekantega
Copy link
Contributor

Okay, not sure what to expect, but here is what it looks like after a fresh pull
Screenshot_20201026-151712_atb

@mikaelbr
Copy link
Collaborator Author

@rikkekantega Yeah, I was about to comment on the color change. I decided to propose the color change across all android devices as I did quick polling with some Android users on what they preferred. I was lazy and the changes shouldn't actually be a part of this PR at all. I can open a new PR if we want to take that discussion separately.

@rikkekantega
Copy link
Contributor

@rikkekantega Yeah, I was about to comment on the color change. I decided to propose the color change across all android devices as I did quick polling with some Android users on what they preferred. I was lazy and the changes shouldn't actually be a part of this PR at all. I can open a new PR if we want to take that discussion separately.

The color change seems fine to me, why not just ship it and see how it works?

@mikaelbr
Copy link
Collaborator Author

Thanks for the screenshot. This is odd. I believe it has something to do with the status bar sizing but it's not consistent across different devices so I'm having a hard time finding a stable solution for this. I think maybe we/you should debug in your device instead of using emulators. (There are some issues with Emulators and notch detection for instance)

@mikaelbr mikaelbr marked this pull request as draft October 26, 2020 14:24
@mikaelbr mikaelbr marked this pull request as ready for review October 27, 2020 07:19
Copy link
Contributor

@rikkekantega rikkekantega left a comment

Choose a reason for hiding this comment

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

Looking good 👍

Copy link
Contributor

@cbrevik cbrevik left a comment

Choose a reason for hiding this comment

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

AWESOME

@mikaelbr mikaelbr merged commit a58c6c1 into master Oct 27, 2020
@mikaelbr mikaelbr deleted the mikael/header-offset-bug branch October 27, 2020 08:11
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

Successfully merging this pull request may close these issues.

3 participants