Skip to content

Feature/1675 dark mode android integration#1717

Merged
marecar3 merged 43 commits intodevelopfrom
feature/1675-dark_mode_android_integration
Mar 19, 2020
Merged

Feature/1675 dark mode android integration#1717
marecar3 merged 43 commits intodevelopfrom
feature/1675-dark_mode_android_integration

Conversation

@marecar3
Copy link
Copy Markdown
Contributor

@marecar3 marecar3 commented Dec 21, 2019 •
edited
Loading

Fixes #1675

Gutenberg PR: WordPress/gutenberg#19293
WordPress-Android PR: wordpress-mobile/WordPress-Android#11004

ezgif com-video-to-gif (13)

To test:

On devices with API <=28:

  1. Open App Settings from Me screen.
  2. Notice the App Theme preference.
  3. The default value should be set to Set by Battery Saver.
  4. From the preference, dialog select Dark and confirm that the app switches to Dark Theme.
  5. Open the Gutenberg Editor and confirm that it has Dark Theme
  6. Select Light and confirm that the app switches to Light Theme.
  7. Open the Gutenberg Editor and confirm that it has Light Theme
  8. Select Set by Battery Saver and enable Battery Saver on the device.
  9. Confirm that Dark Theme is used when Battery Saver is ON.

On devices with API 29+:

  1. Open App Settings from Me screen.
  2. Notice the App Theme preference.
  3. The default value should be set to System Default.
  4. From the preference, dialog select Dark and confirm that the app switches to Dark Theme.
  5. Open the Gutenberg Editor and confirm that it has Dark Theme
  6. Select Light and confirm that the app switches to Light Theme.
  7. Open the Gutenberg Editor and confirm that it has Light Theme
  8. Select System Default and close the app.
  9. Enable Dark Theme in Display settings of the device.
  10. Launch the app again and confirm that the dark theme is applied.

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:

  • replace "future release" in release notes with some version
  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@marecar3 marecar3 mentioned this pull request Dec 21, 2019
Merged
6 tasks
@marecar3 marecar3 mentioned this pull request Dec 21, 2019
Merged
2 tasks
@marecar3 marecar3 requested review from cameronvoell, etoledom, hypest, maxme and mchowning and removed request for cameronvoell and mchowning December 25, 2019 16:21
@marecar3 marecar3 marked this pull request as ready for review December 25, 2019 16:22
@marecar3
Copy link
Copy Markdown
Contributor Author

marecar3 commented Dec 25, 2019 •
edited
Loading

Early feedback from @iamthomasbishop :

  • Android-only: change primary surface background to #121212
  • Link color should be blue 30 (double-check iOS for proper value)
  • When editor is loading, background is white (haven’t seen this in iOS)
  • Bottom sheet icons don't always display as same/correct color (sometimes blueish gray, sometimes neutral. See gallery appender/image sources sheet for example)
  • Slider drag handle is using too dark of a blue (should be same as process fill)

@maxme
Copy link
Copy Markdown
Contributor

maxme commented Dec 31, 2019

Link color should be blue 30 (double-check iOS for proper value)

I noticed the iOS / Android color difference on links:

iOS Android
Simulator Screen Shot - iPhone 11 - 2019-12-31 at 12 06 02 Screenshot_1577790361

etoledom
etoledom approved these changes Feb 17, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@etoledom etoledom left a 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)

mchowning
mchowning reviewed Mar 9, 2020
View reviewed changes
...ava/org/wordpress/mobile/ReactNativeGutenbergBridge/RNReactNativeGutenbergBridgePackage.java Outdated
public class RNReactNativeGutenbergBridgePackage implements ReactPackage {
private GutenbergBridgeJS2Parent mGutenbergBridgeJS2Parent;
private RNReactNativeGutenbergBridgeModule mRNReactNativeGutenbergBridgeModule;
private boolean mIsDarkMode;
Copy link
Copy Markdown
Contributor

@mchowning mchowning Mar 9, 2020 •
edited
Loading

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.

Copy link
Copy Markdown
Contributor Author

@marecar3 marecar3 Mar 12, 2020

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!

@mchowning
Copy link
Copy Markdown
Contributor

mchowning commented Mar 9, 2020

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).

Starting editor with battery Saver OFF Starting editor with battery saver ON
drak_mode_start_light dark_mode_switching_api26.

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.

marecar3 added 10 commits March 10, 2020 19:01
@marecar3
Copy link
Copy Markdown
Contributor Author

marecar3 commented Mar 12, 2020

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!

mchowning
mchowning approved these changes Mar 15, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@mchowning mchowning left a 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)

marecar3
marecar3 commented Mar 19, 2020
View reviewed changes
RELEASE-NOTES.txt
1.24.0
------
* New block: Latest Posts
* [Android] Can now scroll post when in landscape orientation with the soft keyboard displayed
Copy link
Copy Markdown
Contributor Author

@marecar3 marecar3 Mar 19, 2020

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.

@marecar3 marecar3 merged commit e288c45 into develop Mar 19, 2020
@marecar3
Copy link
Copy Markdown
Contributor Author

marecar3 commented Mar 19, 2020

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)

Hey @mchowning, thanks for asking!
@khaykov has opened a new PR wordpress-mobile/WordPress-Android#11469 where we will use a merged hash.

@marecar3 marecar3 deleted the feature/1675-dark_mode_android_integration branch March 20, 2020 09:57
@marecar3 marecar3 mentioned this pull request Mar 20, 2020
Merged
7 tasks
jbinda added a commit that referenced this pull request Mar 24, 2020
@jbinda
Revert "Feature/1675 dark mode android integration (#1717)"
772a66f 
This reverts commit e288c45.
@marecar3 marecar3 mentioned this pull request Mar 25, 2020
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cameronvoell cameronvoell Awaiting requested review from cameronvoell

2 more reviewers

@mchowning mchowning mchowning approved these changes

@etoledom etoledom etoledom approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Dark mode

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Dark Mode - Android integration

4 participants

@marecar3 @maxme @etoledom @mchowning