-
Notifications
You must be signed in to change notification settings - Fork 58
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
Feature/1675 dark mode android integration #1717
Feature/1675 dark mode android integration #1717
Conversation
Early feedback from @iamthomasbishop :
|
# Conflicts: # gutenberg
# Conflicts: # gutenberg
# Conflicts: # gutenberg
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.
JS Side looks good to me! 👍
Let's check with @mchowning or @maxme for a final look on the Native side.
Let's not forget to change the Future release
entry to the proper one we know this will land.
(Today is the last chance to merge to v1.23.0)
@@ -12,19 +12,23 @@ | |||
public class RNReactNativeGutenbergBridgePackage implements ReactPackage { | |||
private GutenbergBridgeJS2Parent mGutenbergBridgeJS2Parent; | |||
private RNReactNativeGutenbergBridgeModule mRNReactNativeGutenbergBridgeModule; | |||
private boolean mIsDarkMode; |
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.
Would be nice if we could mark mIsDarkMode
and mGutenbergBridgeJS2Parent
as final
. Would make it clear that mRNReactNativeGutenbergBridgeModule
is the only thing that can/should change, which also makes it obvious that this is not a dark mode variable that gets updated if the user's dark mode settings change (i.e., battery saver starts). I do not feel strongly about this change though, feel free to leave as-is if you prefer.
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 good advice.
Will update them!
This is looking good @marecar3 ! Noticed that on API<29 devices switching battery mode on/off while within the editor would update the dark mode for the app (i.e. the toolbar on the top would update), but not for the editor. This seems like a bit of an edge case, and I wouldn't mind proceeding with this PR as is and opening a new issue for that (like what we did with the similar issue on API 29+ devices).
I also left a comment about a couple of issues I saw on the WPAndroid PR, but those issues are almost certainly not related to your changes. |
# Conflicts: # gutenberg # react-native-gutenberg-bridge/android/src/main/java/org/wordpress/mobile/WPAndroidGlue/WPAndroidGlueCode.java
# Conflicts: # gutenberg
# Conflicts: # gutenberg
# Conflicts: # gutenberg
Hey @mchowning! Thanks for the good review! I have found where is the problem and now it should be fixed. Please, can you do another iteration of testing? Thanks! |
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.
Looks good! The issue with the editor not immediately switching themes when battery mode is changed is now fixed. Great work @marecar3 !
I did notice that the WPAndroid PR already got merged. Should we open a new WPAndroid PR once this gets merged so that we can update that to use the merged hash? (might not be necessary since the WPAndroid PR is only targetting a feature branch)
@@ -5,10 +5,10 @@ | |||
1.24.0 | |||
------ | |||
* New block: Latest Posts | |||
* [Android] Can now scroll post when in landscape orientation with the soft keyboard displayed |
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 shouldn't be part of release notes as it was removed: https://github.com/wordpress-mobile/gutenberg-mobile/pull/1989/files#diff-a3d054590c32de57d87be233927e755dL3
But for some reason, it was reverted from revert.
Hey @mchowning, thanks for asking! |
Fixes #1675
Gutenberg PR: WordPress/gutenberg#19293
WordPress-Android PR: wordpress-mobile/WordPress-Android#11004
To test:
On devices with API <=28:
Set by Battery Saver
.Dark
and confirm that the app switches to Dark Theme.Light
and confirm that the app switches to Light Theme.Set by Battery Saver
and enable Battery Saver on the device.On devices with API 29+:
System Default
.Dark
and confirm that the app switches to Dark Theme.Light
and confirm that the app switches to Light Theme.System Default
and close the app.Note for API 29+: At this moment we aren't able to get the app theme to switch automatically when the Dark Theme is toggled. For now, you need to restart the app for new theme to take effect. @khaykov has created a separate issue for this at wordpress-mobile/WordPress-Android#10748
PR submission checklist:
RELEASE-NOTES.txt
if necessary.