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

android: styles.xml change for dark theme, on the path to RN v0.64 upgrade. #4846

Merged
merged 1 commit into from
Jul 7, 2021

Conversation

chrisbobbe
Copy link
Contributor

We need to test on Android 10 and record any changes; the linked doc says the dark theme is available in Android 10 (API level 29) and higher. I've tested on the office Android device, a Samsung Galaxy S9, but it's running Android 9 and I'm not sure it's time to upgrade it yet (I'm not sure if we can go back to Android 9 after doing so).

I found no visible changes in the app on Android 9 when switching the system-level dark theme setting on and off. I'm not sure how I was able to switch that setting on and off when the doc says that the Dark theme is available starting in Android 10. Anyway, best to test on Android 10.

@AkashDhiman, would you be interested in testing on a device running Android 10?

Related: #4426
Related: #4009

@AkashDhiman
Copy link
Member

Sure, I will have a look.

@AkashDhiman
Copy link
Member

checked by using ../zulip/tools/reset-to-pull-request 4846 and then launching the app, the usual way.

The following observation is made on Android 10 and 11:

  • white screen that comes up on starting the app changes to darker color when system UI is set to dark mode, this does not happen in the current master build.

There are no visual changes within the app.

For reference here are some screen captures on an Android 10 emulator:

when the system UI is set to light mode

on-sys-light-mode

when the system UI is set to dark mode

on-sys-dark-mode

Copy link
Contributor

@WesleyAC WesleyAC left a comment

Choose a reason for hiding this comment

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

LGTM 👍

As Android's dark theme guide says is necessary [1].

Done now, to follow the template-app change in
facebook/react-native@2b56011f9, on the path to the RN v0.64
upgrade.

The RN commit contains other changes to the template app that we
don't transfer to our project right now:

- The placeholder App.js starts using `useColorScheme` [2], an API
  that was new in RN v0.62. We may want to use this API in some
  places, during or after zulip#4009, but it doesn't seem necessary now.

- On iOS, the root view defined in AppDelegate.m gets its
  `.backgroundColor` set to `[UIColor systemBackgroundColor]` when
  possible. That seems intended to vary the background with the
  system-set theme (dark or light). We don't want to make this
  change: we've so far decided to set `.loadingView` instead of
  `.backgroundColor`, and we do so in a way that explicitly doesn't
  change with the theme (it uses the brand color).

- On iOS, the launch-screen storyboard is changed to adapt to the
  system-set theme. No need to make that change; we've so far chosen
  not to have it change with the theme, and just to use our brand
  color.

[1] https://developer.android.com/guide/topics/ui/look-and-feel/darktheme#support-dark-theme
[2] https://reactnative.dev/docs/usecolorscheme
@chrisbobbe chrisbobbe merged commit 4db472a into zulip:master Jul 7, 2021
@chrisbobbe
Copy link
Contributor Author

Thanks for testing it out, @AkashDhiman, and thanks for the review, @WesleyAC! Merged.

@gnprice
Copy link
Member

gnprice commented Jul 7, 2021

Thanks all!

I'm not sure how I was able to switch that setting on and off when the doc says that the Dark theme is available starting in Android 10.

FWIW my guess for this is that Samsung added a version of this feature for their own devices before it was in Android itself.

I'd expect their version to apply to their own apps, and possibly some third-party apps that explicitly use some Samsung-specific API to get the theme. It probably doesn't succeed in setting the theme in a way that apps relying on the Android 10 API will notice, because they probably designed it before that API existed. If you didn't see an effect like the one Akash showed above, then that fits with that expectation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants