-
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
Fix android status bar color #15471
Fix android status bar color #15471
Conversation
// Make the app take up the full screen (painted behind transparent status bar) | ||
Window w = getWindow(); | ||
w.setFlags(WindowManager.LayoutParams.FLAG_LAYOUT_NO_LIMITS, WindowManager.LayoutParams.FLAG_LAYOUT_NO_LIMITS); |
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.
No need to do this programmatically, it's less obvious and hard to find IMO. Instead, let's use the style attribute (suggested below).
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.
Ah, I take this back. Seems like it is necessary because React Native doesn't use Android layouts.
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.
A problem with React native not letting us modify the Android layout is that adding padding to our outermost layout (to solve the overlapping issue) is problematic. I think we'll need to do this:
- Get the status bar height, we cannot hardcode this because the size varies by device/screen density
- Programmatically set the View padding to the status bar height, using something like this
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.
The PR is on the right track. We have no choice but to use transparency as manually setting the status bar colour at runtime will never happen fast enough to not be noticeable. Here are the things we need to resolve:
- We need to add padding to the top of each page to ensure the content doesn't overlap with the status bar icons (see screenshots)
- We now extend beneath the OS navigation icons, so we'll also need to make sure we don't overlap here. This seems to be the case already, but I noticed that the padding is a bit taller than it needs to be (this is most noticeable on the chat page)
- One exception to the above is the Sign in page, where we do appear to be overlapping the content and OS navigation icons
Overlapping status | Overlapping navigation | Excess navigation padding |
---|---|---|
You might have noticed that we had a separate style for Splash and all other pages. Now that we don't need to worry about hardcoding a status colour we can merge these two and have a single consistent app theme. I'll make some suggestions on your PR |
<item name="android:navigationBarColor">@android:color/transparent</item> | ||
<item name="android:statusBarColor">@android:color/transparent</item> | ||
<item name="android:windowLightStatusBar" tools:targetApi="m">true</item> |
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 unnecessary as BootTheme extends BaseAppTheme
<item name="android:windowTranslucentStatus">false</item> | ||
<item name="android:fitsSystemWindows">true</item> |
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 can remove these now
Seems like we're closing this in favour of @janicduplessis's PR? |
Details
Fixed Issues
$ #13001
Tests
Offline tests
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-20230223-173342.mp4