[Gutenberg] Support Theme Colors and Gradients#12041
Conversation
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
|
You can test the changes on this Pull Request by downloading the APK here. |
|
@mchowning I'm not sure how busy you are with the release stuff these days but I was wondering if you could review this PR for me. It uses the I CC'd @mkevins as a possible backup though if you are busy right now. |
|
👋 @chipsnyder ! Thanks for the ping. Unfortunately I do have a lot of tasks on my plate right now, so I don't really have timento get to this until the later part of this week. It may be a good idea to go ahead and get a review from @mkevins if he is available. Of course, if this is high-priority and needs review ASAP, let me know. FWIW, at a quick glance, what you're doing in these PRs looks reasonable to me. |
|
I have begun reviewing this (and the related PRs), and agree with mchowning that the approach looks good. I have a few groundskeeping items at the moment, so just a heads up that I may be a little delayed in providing more thorough feedback, but please let me know if this needs more urgent attention. |
|
Thanks @mkevins and @mchowning, for helping review this and the other PRs. I'd like to get this in before the next Gutenberg release but I know you have other commitments too. I'd rather you have the time to give your good thorough feedback. |
WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostActivity.java
Outdated
Show resolved
Hide resolved
|
@mkevins I updated your callout good catch on that one. I'm not 100% sure if that was just one comment from reviewing or if that was your full initial review, but that said, I goofed and didn't think about how this PR might block other Gutenberg changes. If you get a chance could you double-check this after these changes and I can tackle anything on Monday morning? If there are any bigger changes though I opened up a draft PR that just puts a default value in place to unblock FYI @marecar3 |
This PR looks good to go, IMO. I had some questions / comments on the FluxC PR, so I guess that's a dependency of this one? I'll review the draft one too, in case the timing is tight (and it's just a stub, so an easy review). |
|
I've reviewed this, and from a code perspective, it looks good. While testing, I encountered some issues for some reason. Tested via https://55113-9306568-gh.circle-artifacts.com/0/Artifacts/WordPress-pr-12041-build-55113.apk on Pixel 3a w/ Android 10. The button gradient did not show when I put the button inside the cover block:
The button did not render when it was placed after the cover block:
I wonder if I'm missing something in the testing steps? I tried with both the twentytwenty theme, and the copied version (where only a single gradient was present in the picker). |
|
Great catch on that @mkevins It looks like something got a bit out of sync on Gutenberg. Opened up a new PR to resolve the button not appearing: WordPress/gutenberg#23075 I found another issue related that where the editor doesn't seem to reload while you're on it. Looking into that now. |
|
@mkevins I figured out the error on this. For some reason when I was generating the bundle the keys after moving them to a constant were reading as null. So I just pulled them out to a new location wordpress-mobile/WordPress-FluxC-Android@9efb81f I also made a tweak in the Gutenberg and updated the dependency here. |
|
Tested again with the Gutenberg changes and the latest FluxC changes. Everything is working as expected now. LGTM! Nice work Chip! |
Thanks for the review @mkevins. I updated the build file for the FluxC beta and bumped the Gutenberg-Mobile version, once the 15.1 branch is cut I think this will be ready to merge. If you could would you be able to hit approve on this either after the branch is cut or in preparation for it? Or if you'd like another set of eyes just let me know :) |
mkevins
left a comment
There was a problem hiding this comment.
If you could would you be able to hit approve on this either after the branch is cut or in preparation for it? Or if you'd like another set of eyes just let me know :)
Not sure why, but I thought I already hit approve. I guess that was the FluxC PR 😅 . Sorry for the delay
Fixes: wordpress-mobile/gutenberg-mobile#1744
Related PRs
gutenbergWordPress/gutenberg#22197gutenberg-mobilewordpress-mobile/gutenberg-mobile#2241WordPress-FluxC-Androidwordpress-mobile/WordPress-FluxC-Android#1593WordPress-iOSwordpress-mobile/WordPress-iOS#14085Description
Adds support for theme defined colors and gradients to be injected into the editor from the mobile apps.
Colors can then be displayed by the same mechanisms that utilize the settings lookup for colors and gradients. This is currently supported on Cover and Button blocks
How has this been tested?
Note Atomic sites don't seem to support the
wp/v2/themesAPI changes yet so this can be tested with a free site or a self-hosted siteTo help with testing this theme can be added to a self-hosted site: twentytwenty-copy.zip
Colors
1.) Select a theme with custom colors (such as TwentyTwenty)
2.) Create a post with blocks that have a custom color set (such as Cover or Button)
3.) Load editor on mobile
Expect to see the custom color on the block
Gradients
1.) Select a theme with custom gradients or add gradients to a theme
2.) Create a post with blocks that have a custom gradient set (such as Cover or Button)
3.) Load editor on mobile
Expect to see the custom gradient on the block
No Wifi
1.) Set up blocks for custom color and gradient
2.) View the post on mobile to cache the theme
3.) Turn off wifi to the device
4.) Reload the editor
Expect to see the custom colors on the blocks
No Wifi - Force kill the app
1.) Set up blocks for custom color and gradient
2.) View the post on mobile to cache the theme
3.) Turn off wifi to the device
4.) Stop the app from running in the background
5.) Reload the editor
Expect to see the custom colors on the blocks
PR submission checklist:
RELEASE-NOTES.txtif necessary.