-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[RNMobile] Support Theme Colors and Gradients #14085
Conversation
… colors to Gutenberg
You can trigger an installable build for these changes by visiting CircleCI here. |
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
let themeSupportStore = StoreContainer.shared.editorTheme | ||
themeSupportQuery = themeSupportStore.query(EditorThemeQuery(blog: post.blog)) | ||
themeSupportReceipt = themeSupportStore.onStateChange { [weak self] (_, state) in | ||
DispatchQueue.main.async { | ||
if let strongSelf = self, let themeSupport = state.editorTheme(forBlog: strongSelf.post.blog)?.themeSupport { | ||
strongSelf.gutenberg.updateTheme(themeSupport) | ||
} | ||
} | ||
} |
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.
I've considered moving these calls outside of the GutenbergViewController. Ultimately I liked it here better because:
- It's closer to the location where it is being used.
- It's a fast enough call to where the average case will have this loaded before the user sees the editor.
- If on a significantly slow network and a user navigates quickly to the editor, then there would still need to be some hook to the Gutenberg editor to call update theme.
- This only matters for the initial load of the editor for a blog, or if the theme changes; because the theme will load from a cache if no changes were detected.
Con:
- There is a hiccup where the event can fire while the editor is still loading. I put a workaround for that issue here.
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.
Can we move this code to an extension file: GutenbergViewController+ThemeSupport
?
And have all the methods associated with theme support moved there?
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.
Yeah that sounds like a good plan.
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.
I ended up moving these to an extension in the same file with a comment. Moving it to a new file would have resulted in changing the access level to a few objects like gutenberg
and the themeSupportQuery
and themeSupportReceipt
which I feel are better left as private
Let me know if this works for you.
Hey @SergioEstevao @pinarol and I thought you might be a good person to ask for in this review when you have time. Not sure how your review plate is right now though so let me know if you want me to ping someone else. |
Testing the following: Button
Cover
No Wifi
No Wifi - Force kill the app
|
guard let responseData = try? JSONSerialization.data(withJSONObject: response, options: []) else { return } | ||
let themeSupports = try? JSONDecoder().decode([EditorTheme].self, from: responseData) | ||
|
||
if let themeKey = EditorThemeStoreState.key(forBlog: blog), |
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.
To make it easier to read this code can we use guard statements instead of having these nested conditions?
|
||
init(from decoder: Decoder) throws { | ||
let map = try decoder.container(keyedBy: CodingKeys.self) | ||
self.colors = try? map.decode([[String: String]].self, forKey: .colors) |
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.
Are we using try?
here because this key can be optional?
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.
Yeah if the colors or gradients are missing then the JSON will have editor-color-palette: false
or editor-gradient-presets: false
so try?
here is to protect against the type change.
It's then left as an optional to make sure we don't send an empty array to Gutenberg and override the default colors that we might want.
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.
Functionality wise it's working as described, I just left some code comments to be addressed.
@SergioEstevao This should be good to go again. I'll handle the Podfile.lock conflict once I have the real commit from the GB side. |
@@ -747,6 +753,10 @@ extension GutenbergViewController: GutenbergBridgeDataSource { | |||
"mentions": post.blog.isAccessibleThroughWPCom() | |||
] | |||
} | |||
|
|||
func gutenbergEditorTheme() -> GutenbergEditorTheme? { |
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.
Could this method move to the extension also?
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.
Yup I can move that. My thought here was that since it's a required method of GutenbergBridgeDataSource
that it belonged with that extension, but I can just leave a comment on the method to help signify that.
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 has been moved now.
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.
Looking good, feel free to merge after the merge conflict is solved.
🙇 Thank you @SergioEstevao |
I'll plan to merge this as soon as the Gutenber Pr's are approved and merged |
Can we delay this merge until next week, we are in the process of cutting a release of GB Mobile and this functionality will not be part of the release. If you wish you can merge it on the after-release branch
…Sent from my iPhone
On 28 May 2020, at 16:14, Chip ***@***.***> wrote:
I'll plan to merge this as soon as the Gutenber Pr's are approved and merged
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
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.
Can we please target the gutenberg/after_1.29.0
branch or wait for next week after the release 1.40 is cut from develop?
Yup that shouldn't be a problem to wait. I'm still working on getting reviews for the other layers any way. |
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.
All good!
Fixes: wordpress-mobile/gutenberg-mobile#1744
Related PRs
gutenberg
WordPress/gutenberg#22197gutenberg-mobile
wordpress-mobile/gutenberg-mobile#2241Description
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/themes
API 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
Screenshots
Types of changes
New feature: Theme support
PR submission checklist:
RELEASE-NOTES.txt
if necessary.