-
Notifications
You must be signed in to change notification settings - Fork 3k
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 janky Android splash screen #12642
Conversation
Hmm, looks like pullerbear is frozen here or something |
Hopefully because of the PR being a WIP? Let's see what happens when the hold is removed. |
@aimane-chnaif @stitesExpensify 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] |
Wow, Melvin already knows I would be a reviewer 😄 |
Hey @stitesExpensify & @aimane-chnaif. This is on hold, but Id would be great to get an initial code review so that I can very quickly merge this after the dark mode change PR is merged (likely today/tomorrow). The dark mode/light mode test is only possible after I have re-merged main. |
# Conflicts: # src/components/CustomStatusBar/index.android.js
@aimane-chnaif @stitesExpensify ignore my last message. I merged the dark mode PR so this is fully testable now. We don't really need to care about light mode right now and I created a follow up issue for this 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.
Testing this!
import themeColors from '../../styles/themes/default'; | ||
import {StatusBar} from 'react-native'; |
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.
@Julesssss There is a linter issue here
Reviewer ChecklistI have tested on Android and iOS since this code only changes Android specific codebase and also the test steps mentioned to check iOS
Screenshots/VideosWebMobile Web - ChromeMobile Web - SafariDesktopAndroidappPull12642Android.mov |
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!
I think we can push this one forward so merging. |
@mountiny looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
Not an emergency, the Jobs failing should be fixed here #13078 |
@Julesssss on devices with embedded bottom navigation bar and in device light mode, screen doesn't look good device dark mode: device light mode: |
oh @mountiny too early to merge |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Hey @aimane-chnaif thanks for raising that, I'll look into that as a separate issue. |
ok I suggest to add navigationBarColor in xml attribute (same color as status bar maybe?) |
Yeah, this is a simple attribute fix but I'm going to keep it internal. Issue here. |
Oh apologies, thanks for testing @aimane-chnaif I think we can look at these quirks in new issues to just keep things going forward |
1 similar comment
@Julesssss @mountiny I also found another issue while testing. composer input is hidden by keyboard for the height of status bar |
🚀 Deployed to production by @luacmartins in version: 1.2.33-7 🚀
|
🚀 Deployed to production by @luacmartins in version: 1.2.33-7 🚀
|
CC @Luke9389
Details
Fixes an issue where the Android Splash screen icon jumped up and down on app open. This introduces a new issue with the status bar colour, but this is being handled separately here.
Fixed Issues
$ #8236
Tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
filesWaiting 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)PR Reviewer Checklist
The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesWaiting 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
have been tested & I retested again)/** 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)Screenshots
Only Android code was modified
Android
device-2022-11-28-122609.mp4
Screenshots
Dark mode
Android
Android/mWeb
iOS
iOS/mWeb
Web
Desktop
Light mode
Android
Android/mWeb
iOS
iOS/mWeb
Web
Desktop
Videos
Android 8.1 - Nexus 5x
https://user-images.githubusercontent.com/10736861/201134924-5ba9d492-78a3-4bce-8a06-7b0c6b3fa151.mp4
Android 13 - Pixel 6A
Still seems a bit janky, need to investigate further
https://user-images.githubusercontent.com/10736861/201135214-d9657e02-7c3e-4c7f-b000-46133e37f5d2.mp4