-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Make status bar transparent on android #15778
Conversation
97e7bab
to
5da0715
Compare
@Julesssss @mananjadhav One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Nice! I'll test this tomorrow 👍 |
5da0715
to
bf1ef9b
Compare
Added one more fix in SetPassword screen to use SafeAreaView from react-native-safe-area-context instead of react-native. The flicker I noticed (#15778 (comment)) only seems to happen on initial app render, the SafeAreaView on SetPassword screens works fine and doesn't flicker. |
@roryabraham Updated with your feedback. |
e175b6c
to
624b378
Compare
@janicduplessis @roryabraham @Julesssss Are we only focus on the status bar or also the bottom bar? For iOS it is transparent at the bottom too. This can be achieved with |
@mananjadhav My plan is to start with statusbar and work on navigation bar after. Navigation bar is a bit more tricky since it prevents automatic keyboard handling that we currently rely on on Android. It requires using KeyboardAvoidingView like on iOS. Some code also kind of assume bottom insets if just rounded corners / gesture area like on iOS instead of being a solid bar like on most android devices. So i figured it would be best to address in a follow up. |
Makes sense, and agreed navigation has its fair set of issues to be dealt with especially as it draws the activity behind the status bar. |
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.
Except for one comment on withWindowDimensions
support for RNW, I think this looks good.
Other comment on bottom bar is already addressed.
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.
This is looking better, but unfortunately I'm seeing a lot more jankiness with these changes. Notice the transition between signin/password and show/hide keyboard. We'll need to fix these too.
Both of these are release builds Current screen-20230313-140509.mp4With changes screen-20230313-140234.mp4 |
Thanks for testing @Julesssss , I suspect this is caused by a different behaviour of frame in react-native-safe-area-context which changes value when keyboard opens vs Dimensions module which doesn’t. I will see how this could be fixed. |
@Julesssss @aimane-chnaif Done! |
Thanks @janicduplessis. Please don't amend commits and force-push once the PR is in review though. |
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.
Kicked off a new build here
ok so expected bug (android splash logo position flickering) happening (around 5s-6s in video below): Screen.Recording.2023-05-24.at.7.49.33.AM.mov |
Hey @aimane-chnaif, what device and OS version is that screencast from? |
happening on all my android devices Above screenshot is from Pixel XL API 33 emulator |
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.
Damn, yeah I'm also seeing this again:
Android 8
Nexus5X.mp4
Nexus5XSignedIn.mp4
Probably need to make statusbar transparent on splash too, will have a look today. |
3b38058
@Julesssss @aimane-chnaif I just pushed a fix for the new flicker issue. |
Please fix lint |
Oups, there you go! |
@Julesssss let's trigger build once again |
New CI build in progress |
🧪🧪 Use the links below to test this build in android and iOS. Happy testing! 🧪🧪
|
Latest version tests well. Small concern about navigation bar (not a blocker): Screen.Recording.2023-05-25.at.7.38.46.AM.movXRecorder_25052023_074403.mp4 |
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.
Thanks for the great work!
I think all good to merge
Hey @aimane-chnaif, thanks for retesting. Good spot, but I think we can live with this for now |
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.
Excellent, thanks again @janicduplessis
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/Julesssss in version: 1.3.19-0 🚀
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.19-7 🚀
|
Details
Make the status bar transparent on Android.
This also fixes 2 issues that were discovered during QA and caused the initial PR (#15778) to be reverted:
Fixed Issues
$ #13001
$ #15770
PROPOSAL: GH_LINK_ISSUE(COMMENT)
Tests
Offline tests
N/A
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android
Screen.Recording.2023-03-09.at.12.41.55.mov