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.
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
Add color and typography presets to Global Styles #56622
Add color and typography presets to Global Styles #56622
Changes from all commits
3366eca
9532adf
ddf005d
f075c7f
92167fd
379f62d
c62be75
1a3dd8a
5f50937
de565ad
db8fc25
76f9a9b
8663e68
e2c18ec
32723f1
baab507
d8ec720
0e94cbe
e6d2392
dcf7f89
e8033c5
b4b0f80
f156dd5
00726ba
f401473
c0be163
1e15d41
a23a6ca
13bea45
63b85db
c17157a
3af2f95
f12bb72
da353ae
88c8a3f
993a98e
ecffb64
9493d20
d368393
7bea0bf
6fbd619
949f199
993c4af
e4eedf0
c642d19
e51b44f
b23d64c
f7b735e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 is a duplicate of the preview component
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.
While testing I noticed an issue with some themes that had only 2 palette colors, e.g., Freddie.
If those colors match heading or background colors, they're being filtered out:
gutenberg/packages/edit-site/src/components/global-styles/hooks.js
Lines 68 to 71 in f7b735e
The result is that, although there's a variation, the colors are empty:
CanvasLoader already gets around it this way:
gutenberg/packages/edit-site/src/components/canvas-loader/index.js
Lines 19 to 23 in f7b735e
Not sure what's appropriate in
<StylesPreviewColors />
Maybe something like:
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.
Update: this is already an "feature" of trunk, in that style variation previews won't show them either.
I think it just looks strange in the preset previews. Maybe good for an immediate follow up.
To replicate the above issue, here's a style variation example. The palette colors are the same as the background and text colors so the variation will show nothing.
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.
Is this animation block necessary any more for these specific previews?
2024-02-26.12.42.40.mp4
If not, wondering if we can remove it entirely. I was going to, but just wanted to make sure it's doing something that's not obvious.
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.
Sorry, didn't see the transition as the panel opens, as with Typography
https://github.com/WordPress/gutenberg/pull/56622/files#r1503522214
A lot of code though for how much value?
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.
If we don't require motion, then I think the
isFocused
prop is also unnecessary (?)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 added this to ensure the colors have a border like the ColorIndicator component.
In some themes, the background color matches the highlights, or are close in contrast:
Before
After
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 don't think this is necessary, as I want to update these colors to be more representative of the style by using the text and button colors for the indicators. I will open another issue on that (to do as a follow-up).