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.

Sign up for GitHub

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

Jump to bottom

[Mobile] - Theme colors sync #26821

Merged
merged 6 commits into from
Nov 17, 2020
Merged

[Mobile] - Theme colors sync #26821

merged 6 commits into from
Nov 17, 2020

Conversation

geriux
Copy link
Member

@geriux geriux commented Nov 9, 2020 •
edited
Loading

Gutenberg Mobile PR -> wordpress-mobile/gutenberg-mobile#2788
WordPress iOS PR -> wordpress-mobile/WordPress-iOS#15258
WordPress Android PR -> wordpress-mobile/WordPress-Android#13327

Description

Fixes an issue with theme colors not syncing correctly.

After some tests, it seems like the code removed in this PR was replacing the updated theme colors with the previous ones, this was introduced here so I wonder if after the useEditorFeature( 'color.palette' ) feature was introduced this is not needed anymore.

Update: So I investigated a bit more and the issue was that passing down the colors/gradients as props were generating another update of the settings in the state triggered after the theme colors callback hence replacing the new colors with the old ones.

I updated it to set the initial colors using updateSettings instead of passing them as props to prevent them from replacing newer color updates in the state.

I went through the test cases of it and it looks like it's working as expected. @chipsnyder I'll add you as a reviewer because you'll have more insight related to this, am I missing something related to the theme colors that still requires this code?

How has this been tested?

Steps to test updating the theme from the app
  • Using the WordPress app
  • Go to Themes and activate Twenty Twenty
  • Create a post
  • Add a cover block
  • Expect to see the following colors:
  • Save the post and close it
  • Go to Themes and activate Barnsbury
  • Open the previously created post
  • Expect to see the following colors:
Steps to test updating the theme from the web
  • Using the WordPress app
  • Create a post
  • Add a cover block (don't select an image/background color), save and close it
  • Close the app (leaving it in the background is fine)
  • On the web, go to your site and set the theme Stow
  • Open the app
  • Open the previously created post that has a Cover block
  • Expect to see the following colors:
  • Close the post
  • Close the app (leaving it in the background is fine)
  • On the web, go to your site and set the theme Brompton
  • Open the app
  • Open the previously created post that has a Cover block
  • Expect to see the following colors:
Steps to test Editor Theme

Editor Theme - 1

  • Default Colors - Check that default colors still load - steps
  • Default Gradients - Check that default gradients still load - steps
  • Custom Colors - Check that custom colors load in the editor - steps

Editor Theme - 2

  • Custom Gradients - Check that custom gradients load in the editor - steps
  • Offline Support - steps

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@geriux geriux added [Status] In Progress Tracking issues with work in progress Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) labels Nov 9, 2020
@github-actions
Copy link

github-actions bot commented Nov 9, 2020 •
edited
Loading

Size Change: 0 B

Total Size: 1.19 MB

ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.77 kB 0 B
build/api-fetch/index.js 3.42 kB 0 B
build/autop/index.js 2.84 kB 0 B
build/blob/index.js 664 B 0 B
build/block-directory/index.js 8.71 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 133 kB 0 B
build/block-editor/style-rtl.css 11.3 kB 0 B
build/block-editor/style.css 11.3 kB 0 B
build/block-library/editor-rtl.css 8.91 kB 0 B
build/block-library/editor.css 8.92 kB 0 B
build/block-library/index.js 147 kB 0 B
build/block-library/style-rtl.css 8.1 kB 0 B
build/block-library/style.css 8.1 kB 0 B
build/block-library/theme-rtl.css 792 B 0 B
build/block-library/theme.css 793 B 0 B
build/block-serialization-default-parser/index.js 1.87 kB 0 B
build/block-serialization-spec-parser/index.js 3.06 kB 0 B
build/blocks/index.js 48 kB 0 B
build/components/index.js 171 kB 0 B
build/components/style-rtl.css 15.3 kB 0 B
build/components/style.css 15.3 kB 0 B
build/compose/index.js 9.9 kB 0 B
build/core-data/index.js 14.8 kB 0 B
build/data-controls/index.js 821 B 0 B
build/data/index.js 8.74 kB 0 B
build/date/index.js 11.2 kB 0 B
build/deprecated/index.js 768 B 0 B
build/dom-ready/index.js 571 B 0 B
build/dom/index.js 4.92 kB 0 B
build/edit-navigation/index.js 11.1 kB 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.51 kB 0 B
build/edit-post/style.css 6.49 kB 0 B
build/edit-site/index.js 23.3 kB 0 B
build/edit-site/style-rtl.css 3.98 kB 0 B
build/edit-site/style.css 3.98 kB 0 B
build/edit-widgets/index.js 26.3 kB 0 B
build/edit-widgets/style-rtl.css 3.16 kB 0 B
build/edit-widgets/style.css 3.16 kB 0 B
build/editor/editor-styles-rtl.css 476 B 0 B
build/editor/editor-styles.css 478 B 0 B
build/editor/index.js 42.5 kB 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.85 kB 0 B
build/element/index.js 4.62 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/index.js 6.86 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.16 kB 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 698 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.1 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.32 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 790 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/reusable-blocks/index.js 3.05 kB 0 B
build/rich-text/index.js 13.3 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.05 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

This was referenced Nov 9, 2020
Merged
Closed
Closed
@geriux geriux removed the [Status] In Progress Tracking issues with work in progress label Nov 10, 2020
@geriux geriux requested a review from chipsnyder November 10, 2020 13:44
@geriux geriux marked this pull request as ready for review November 10, 2020 13:45
chipsnyder
chipsnyder reviewed Nov 10, 2020
View reviewed changes
Copy link
Contributor

@chipsnyder chipsnyder left a comment

Choose a reason for hiding this comment

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

Hey @geriux, Thanks for taking this on!

I wonder if after the useEditorFeature( 'color.palette' ) feature was introduced this is not needed anymore.

I think you're right that this feature is what ended up breaking this flow. I ran through the test scenarios and the only one that didn't work was the offline support (which is what would have come through on these call paths).

These calls are actually here to help populate the cached data from the device. What I'm seeing while testing Android is:

  • Select a theme
  • Check the editor.
  • Expect to see the colors and gradients for the theme
  • leave the editor.
  • Check the editor again
  • The colors have reverted to the default options.

https://cloudup.com/cioMC-zdXfD

@geriux
Copy link
Member Author

geriux commented Nov 13, 2020

Hey @chipsnyder ! Thank you for testing and for explaining why that code was added =)

So I investigated a bit more and the issue was that passing down the colors/gradients as props were generating another update of the settings in the state triggered after the theme colors callback hence replacing the new colors with the old ones.

I updated it to set the initial colors using updateSettings instead of passing them as props to prevent them from replacing newer color updates in the state.

I updated the builds so you can give it another test when you have time. Thanks!

@geriux geriux requested a review from chipsnyder November 13, 2020 16:20
chipsnyder
chipsnyder approved these changes Nov 13, 2020
View reviewed changes
Copy link
Contributor

@chipsnyder chipsnyder left a comment

Choose a reason for hiding this comment

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

Awesome job tracking this down @geriux! Thanks for adding in the description of how the bug was introduced. Tested this out on iOS and Android via the provided builds and it works great!

@geriux geriux merged commit 2926884 into master Nov 17, 2020
@geriux geriux deleted the rnmobile/fix/theme-color-sync branch November 17, 2020 10:02
@github-actions github-actions bot added this to the Gutenberg 9.5 milestone Nov 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hc0503 hc0503 hc0503 approved these changes

@chipsnyder chipsnyder chipsnyder approved these changes

Assignees
No one assigned
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)
Projects
None yet
Milestone
Gutenberg 9.5
Development

Successfully merging this pull request may close these issues.
3 participants
@geriux @chipsnyder @hc0503