-
-
Notifications
You must be signed in to change notification settings - Fork 654
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
Conversation
453e551
to
2d40d5d
Compare
Sure, I will have a look. |
checked by using The following observation is made on Android 10 and 11:
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 modewhen the system UI is set to dark mode |
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.
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
2d40d5d
to
4db472a
Compare
Thanks for testing it out, @AkashDhiman, and thanks for the review, @WesleyAC! Merged. |
Thanks all!
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. |
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