Fix Custom Palette colors and support multiple origins and theme cache issues. #38417
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ne to Aztec, the color parser throws an error if its invalid.
geriux
added
[Status] In Progress
|
Size Change: +549 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
geriux
changed the title
geriux
added
the
Mobile App - Automation
geriux
force-pushed
the
rnmobile/fix/invalid-link-colors
branch
from
b29ab8a to
b542e4e
Compare
8 tasks
fluiddot
reviewed
Feb 3, 2022
There was a problem hiding this comment.
Thanks @geriux for addressing this issue 🙇 . I tested the changes and so far I encountered the following issues, I'd say that none of them are critical but it would be interesting to check them further:
Block-based themes - Highlight text within a Button block is not preserved when saving
Steps:
- Add a Buttons block.
- Type some text within a Button block.
- Select part of the text.
- Tap on the Highlight text color button (Button with
Aicon). - Select a color.
- Observe that color of the selected text changed to previously picked.
- Save the post/page.
- Observe that the color of the selected text changes back to its original color.
NOTE: I couldn't reproduce this issue either in version 19.0 or the web version.
Standard themes - Default colors are reverse ordered
Steps:
- Add a Paragraph block.
- Open the block settings.
- Tap on the Text option.
- Observe that the colors are not ordered as listed here.
NOTE: I couldn't reproduce this issue in version 19.0.
Click here to display screenshot
| Before | After |
|---|---|
 |
 |
packages/components/src/mobile/global-styles-context/utils.native.js
Outdated
Show resolved
Hide resolved
packages/components/src/mobile/global-styles-context/utils.native.js
Outdated
Show resolved
Hide resolved
packages/components/src/mobile/global-styles-context/utils.native.js
Outdated
Show resolved
Hide resolved
packages/components/src/mobile/global-styles-context/utils.native.js
Outdated
Show resolved
Hide resolved
Comment on lines
1019
to
1026
| getLinkTextColor( defaultColor ) { | ||
| const { style } = this.props; | ||
| const customColor = style?.linkColor && colord( style.linkColor ); | ||
|
|
||
| return customColor && customColor.isValid() | ||
| ? customColor.toHex() | ||
| : defaultColor; | ||
| } |
There was a problem hiding this comment.
Now that the var:preset values are parsed via parseStylesVariables when getting the global styles, probably we no longer need to have this check, wdyt?
There was a problem hiding this comment.
I think we should keep it, if a user sets something like #fff for the links within the theme.json it would generate a crash on Android 😅
fluiddot
reviewed
Feb 4, 2022
There was a problem hiding this comment.
I checked further the issue "Default colors are reverse ordered" (reference) and I'm getting different results by platform, site type and themes 🙃. I'm not sure what's causing it but it would be great to investigate it.
Simple site - Block-based theme (with a custom palette) ❌
Theme colors:
| iOS | Android |
|---|---|
 |
 |
iOS:
- Colors are not inverted but the custom palette is placed after the theme palette ❌ .
- Colors displayed in the Cover block are inverted ❌ .
Android:
- Colors are displayed in correct order ✅ .
- Colors displayed in the Cover block are inverted ❌ .
Simple site - Standard theme (Seedlet theme) ❌
| iOS | Android |
|---|---|
 |
 |
iOS:
- Colors are inverted ❌ .
- Colors displayed in the Cover block are inverted ❌ .
Android:
- Colors are displayed in correct order ✅ .
- Colors displayed in the Cover block are not the first ones from the palette ❌ .
Self-hosted - Standard theme (Seedlet theme) ❌
| iOS | Android |
|---|---|
 |
 |
iOS:
- Colors are inverted ❌ .
- Colors displayed in the Cover block are not the first ones from the palette ❌ .
Android:
- Colors are displayed in correct order ✅ .
- Colors displayed in the Cover block are not the first ones from the palette ❌ .
|
Hey @fluiddot Thank you so much for the review and testing! I'll start looking at the feedback and addressing your comments! 🙇 |
added 2 commits
February 7, 2022 11:57
# Conflicts: # packages/rich-text/src/component/index.native.js
added 4 commits
February 8, 2022 14:01
Thanks for testing it out @iamthomasbishop! I added an extra of 8px:
What do you think? Thanks again! |
|
New builds are available: |
fluiddot
reviewed
Feb 11, 2022
There was a problem hiding this comment.
I've done another round of tests following the test instructions on both platforms using the installable builds, and everything works fine ✅, awesome work @geriux 🥇!
I also confirm that the issue "Color tabs disappear when selecting a color and navigating between Solid and Gradient" is addressed with the recent commits 🎊 .
Apart from the two comments I added, everything is ready from my side to consider the approval of this PR.
packages/components/src/mobile/global-styles-context/utils.native.js
Outdated
Show resolved
Hide resolved
packages/components/src/mobile/bottom-sheet/bottom-sheet-navigation/navigation-screen.native.js
Show resolved
Hide resolved
|
@geriux that looks better, thank you! |
added 2 commits
February 14, 2022 14:33
… not an empty array
# Conflicts: # packages/react-native-editor/CHANGELOG.md
fluiddot
approved these changes
Feb 14, 2022
There was a problem hiding this comment.
I verified that the latest comments have been addressed, I also performed a quick test locally as a smoke test since I fully tested the PR when doing this review. On the other hand, I added a minor comment on the code, but feel free to omit it if you wish.
LGTM 🎊 !
…and defaultEditorGradients
2 tasks
cbravobernal
added
[Type] Bug
cbravobernal
changed the title
4 tasks
SiobhyB
pushed a commit
that referenced
this pull request
Mar 3, 2022
…heme cache issues (#38417) * Mobile - Rich Text - Validate link colors before passing an invalid one to Aztec, the color parser throws an error if its invalid. * Remove unneeded check * Mobile - Global styles - Parse custom palette colors e.g var:preset|color|custom-color-2 * Mobile - Global styles - Update tests to take into account custom colors * Mobile - Adds useMobileGlobalStylesColors hook to get global styles palette colors * Mobile - Update Cover and Global styles test * Mobile - Cover - Use memo for the inline color palette * Mobile - Cover - Revert test change due to colors update * Mobile - Add support for multiple color palettes and gradients including custom ones * Mobile - Rich Text - Simply color palettes merge * Mobile - Global styles - Enable default palette and default gradients if no value comes from the API * Mobile - Update CHANGELOG * Mobile - Global styles utils - Update test with default values * Mobile - Provider - Fix replacing previous colors on settings update * Mobile - Enable text and color settings since we are providing default colors when there's no theme data available * Mobile - Android - Fix issue where a restart was needed to load the theme colors * Mobile - Reset Global styles base styles for standard themes. Fixes sync issues when changing between themes. * Mobile - Palette screen - Add top and bottom padding for the container of the color palettes * Mobile - Bottom sheet navigation screen - Add missing dependencies in useMemo * Mobile - Global styles - useMobileGlobalStylesColors: use the type to get the fallback colors/gradients * Mobile - Provider - Always return getColorsAndGradients if it's not a block theme * Mobile - Global styles - Enable default color/gradient palette if its not an empty array * Mobile - Global styles - Set default values for defaultEditor colors and defaultEditorGradients
|
Hi @geriux! I'm trialing for the Penguin team (QE) and I was trying to set up one site to reproduce the testing scenarios described in the PR, but I do not see the Just some extra info, I was trying with a block-based theme (my site is using Quadrat Yellow). Any help will be very welcome! Thank you! |
|
Hey @brcavallaro 👋
This option is no longer available because Full site editing is no longer opt-in and it's enabled automatically once you use a block-based theme, like Quadrat, I'll edit the PR description to reflect that. You should have the editor enabled already by going to Appearance > Editor (beta). Could you try if you can see that option? Thanks! |
Hi @geriux 👋 So sorry about the looong delay without replying here. But just FYI, I can see the option you mentioned, so we're all good now :) Thank you very much for the support! |
This was referenced Jan 5, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Gutenberg Mobile PR-> Mobile - Fix Custom Palette colors and support multiple origins wordpress-mobile/gutenberg-mobile#4537WordPress iOS PR-> [TEST] - Gutenberg - Fix Custom Palette colors and support multiple origins and theme cache issues wordpress-mobile/WordPress-iOS#17910WordPress Android PR-> [TEST] - Gutenberg - Fix Custom Palette colors and support multiple origins and theme cache issues wordpress-mobile/WordPress-Android#15913Fixes #38525 and #36152
This is a follow-up of #38474
Description
This PR fixes several issues, first, it fixes an ongoing issue where changing between themes would not refresh the current theme data and a restart of the editor was needed, as well as sometimes opening the editor for the first time and not seeing the theme colors.
Second, it adds support for multiple color and gradient palettes including the
Customones.Now both standard and block-based themes get the color data from these multiple origin palettes, so this PR removes setting the
colorandgradientson the top-level of the editor settings. Including the validate colors helpers.The palette screen now supports rendering multiple palettes coming from
useMultipleOriginColorsAndGradients. These areTheme,Default, andCustom(if there's one).On
global-styles-context/utils.native.jsit now supports parsing color variables likevar:preset|color|custom-color-2this is the case for custom colors.It introduces
useMobileGlobalStylesColorswhich merges all colors from all palettescolors/gradientsto get all available colors in one array.It also introduces
getColorsAndGradientswhich gets the default editor color/gradients for standard themes. It has a fallback just in case we can't get the data from the APIrawFeaturesso it sets the default colors that are built-in within the editor.On Android, when
subscribeUpdateEditorSettingsis called to update the theme data, it wasn't passing therawFeaturesdata, only when the editor was opened inside theinitialProps. This was causing issues like the editor not showing the active theme colors.These changes affect quite a few files where colors are used:
Testing Instructions - Block-based themes e.g. (Calvin, Quadrat)
Test case 1 (Multiple origin color palettes)
Precondition: Add a custom palette to your theme
Enable Full site editing: On WordPress.com go to Settings > Full site editing (beta)Edit: This is no longer opt-in so now it's enabled by default.
TextTheme,DefaultandCustom(in that order)Test case 2 (Multiple origin gradient palettes)
Precondition: A custom gradient palette, use the same steps from the previous test case but for gradients.
BackgroundGradientsegmentDefaultandCustom(in that order). If your theme has built-in gradients, theThemepalette should show up as well.Test case 3 (Test different blocks)
Testing Instructions - Standard themes e.g. (Seedlet, Twenty Twenty-One)
Test case 1 (Multiple origin color palettes)
TextTheme, andDefaultTest case 2 (Multiple origin gradient palettes)
BackgroundGradientsegmentThemeandDefaultTest case 3 (Test different blocks)
Testing Instructions - Changing between themes
Test case 1
Screenshots
Block-based themes e.g. (Calvin, Quadrat)
Standard themes e.g. (Seedlet, Twenty Twenty-One)
Types of changes
Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).