-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Conversation
Size Change: +1.64 kB (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
Flaky tests detected in b63b417. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8007146955
|
e00a931
to
a39c4c8
Compare
|
||
const { GlobalStylesContext } = unlock( blockEditorPrivateApis ); | ||
|
||
const getFontFamilies = ( themeJson ) => { |
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.
There's a lot of duplicate code in this function that could be extracted to a function.
}; | ||
|
||
export default function ScreenTypographyTypesets() { | ||
const variations = useSelect( ( select ) => { |
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.
Return an object.
const { base, user } = useContext( GlobalStylesContext ); | ||
|
||
const typographyVariations = | ||
variations && getVariationsByProperty( user, variations, 'typography' ); |
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.
Add the default?
} ); | ||
}; | ||
|
||
const selectOnEnter = ( event ) => { |
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.
Quite a lot of this is just copy/paste from the Variation component.
|
||
export default function ColorVariations() { | ||
const { user } = useContext( GlobalStylesContext ); | ||
const variations = useSelect( ( select ) => { |
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.
Return an object
).__experimentalGetCurrentThemeGlobalStylesVariations(); | ||
}, [] ); | ||
const colorVariations = | ||
variations && getVariationsByProperty( user, variations, 'color' ); // should also get filter? |
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.
Add the default variation
} | ||
|
||
export default function TypographyVariations() { | ||
const variations = useSelect( ( select ) => { |
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.
Return an object
|
||
const { base, user } = useContext( GlobalStylesContext ); | ||
|
||
const typographyVariations = |
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.
Add the default variation
c0616c6
to
98a2dad
Compare
This doesn't seem to be working for variations with more than 2 fonts. I added this variation to TT4 that uses a third font for buttons and it didn't show under typesets:
|
@scruffian @MaggieCabrera If you need help moving this one forward, I'm happy to rebase and test, and tinker with it. I'll take it for a spin next week. If there are any outstanding items or gotchas, let me know. Thank you! |
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
||
typographyVariations?.forEach( ( variation ) => { | ||
const [ bodyFontFamilyName, headingFontFamilyName ] = | ||
getFontFamilyNames( mergeBaseAndUserConfigs( base, variation ) ); |
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.
Maybe we need to come up with a way to name style variations with no font-families, e.g., cases where variations have typography styles only like this:
{
"settings": {},
"styles": {
"typography": {
"letterSpacing": "10px"
},
"blocks": {
"core/post-title": {
"typography": {
"fontWeight": "700"
},
"color": {
"background": "red"
}
}
}
}
}
Possibly ${variationTitle} - typeset
or, when variation.title
doesn't exist in the theme.json ${variationFileName} - typeset
?
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.
Maybe we need to come up with a way to name style variations with no font-families, e.g., cases where variations have typography styles only like this:
To simplify this, I propose we lean in on a "Font book" type approach, where the heading and body fonts are used in for A
and a
respectively, like this:
We already do that for styles:
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.
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, this looks good. We can expand later on listing exactly what aspects are being modified, which is a general concern of variations.
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.
Looks great!
98a2dad
to
cde7009
Compare
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.
Just rebasing/testing today and making some observations. I can look further into the following:
- create hooks to grab typography/color variation sets
- decide how to handle labelling variations with no presets
- tests
- other stuff 😄
<VStack spacing={ 3 }> | ||
<FontFamilies /> | ||
<ItemGroup isBordered> | ||
<NavigationButtonAsItem |
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.
Wondering if the all the work grabbing the typography settings from the variations could be done in a memoized hook, or store selector.
That way we could do a length check on the array here and skip rendering the button.
packages/edit-site/src/components/global-styles/variations-color.js
Outdated
Show resolved
Hide resolved
I'm thinking we could split this PR. First we could work on the hook to grab theme style variation styles by property: This could more quickly unblock the work @apeatling is doing. This would mean we'd work on the UI/site editor components etc in another PR. It would further help to split up the PR as there are some aspects of how and what we show in the editor that I think need resolved. For instance how should the site editor, if at all, present theme style variations that have no presets? Related comments: https://github.com/WordPress/gutenberg/pull/56622/files#r1477640893 and https://github.com/WordPress/gutenberg/pull/56622/files#r1477627148 Maybe we shouldn't focus on presets so much and rather present them as subsets of variations (typography and so on) So if a theme hasn't got a any heading/body font families defined, what do we do? In short, in my estimation, the discussions about UX and UI will take longer than getting some working utils ready. What do folks think? |
I think they're one and the same. Having a full theme style variation is a nice way to switch up your whole site's look, but splitting out the colors and typesets within each is nice so you're not having to go all-or-nothing.
I wouldn't expect there to be a preset for it then. |
Thanks for the feedback @richtabor! This gives us plenty to go off, thanks! |
I agree with this. There's a whole lot of crazy javascript manipulations happening that need to be tidied up and tested thoroughly. We can get a lot of this in place before making the UX decisions. Thanks for working on this. |
packages/edit-site/src/components/global-styles/font-library-modal/utils/preview-styles.js
Show resolved
Hide resolved
Pulling out the utils from #56622 so we test separately
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 working pretty well, thanks a lot!
I'm happy if @mtias and @richtabor are happy. 😄
I've tested in about 12 block themes and didn't notice any huge blockers.
For follow ups
It would be good to have some fallbacks for highlightedColors in case the background/text styles are the same as the available palette colors. See comment: #56622 (comment)
From this comment: Might also be a good janitorial follow up to move variation components into components/global-styles/variations and then all the related CSS as well
My feeling is that we should probably refactor the StylePreview component into smaller pieces that can be reused, so that all three previews share the same code, but this will take a bit of unpicking...
Agree. This would be a good refactor follow up.
*/ | ||
const ratio = ratioState ? ratioState : fallbackRatio; | ||
|
||
const { highlightedColors } = useStylesPreviewColors(); |
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
const highlightedColors = paletteColors | |
.filter( | |
// we exclude these two colors because they are already visible in the preview. | |
( { color } ) => color !== backgroundColor && color !== headingColor |
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
const [ fallbackIndicatorColor ] = useGlobalStyle( 'color.text' ); | |
const [ backgroundColor ] = useGlobalStyle( 'color.background' ); | |
const { highlightedColors } = useStylesPreviewColors(); | |
const indicatorColor = | |
highlightedColors[ 0 ]?.color ?? fallbackIndicatorColor; |
Not sure what's appropriate in <StylesPreviewColors />
Maybe something like:
const { paletteColors, highlightedColors } = useStylesPreviewColors();
const previewColors = highlightedColors.length
? highlightedColors
: paletteColors.slice( 0, 2 );
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.
{
"version": 2,
"settings": {
"color": {
"palette": [
{
"color": "#000000",
"name": "Base",
"slug": "base"
},
{
"color": "#ffffff",
"name": "Contrast",
"slug": "contrast"
}
]
}
},
"styles": {
"color": {
"text": "#000000",
"background": "var(--wp--preset--color--contrast)"
}
}
}
key={ slug } | ||
style={ { | ||
boxShadow: | ||
'inset 0 0 0 1px #0003', |
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.
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.
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).
I agree, hierarchy wise they're a bit in the mix—rather than appropriately placed. I think this could work for this initial effort, then we can revisit secondary affordances for when there are more than 6 (perhaps a "View more/all" and a flyout, like the Colors panel): |
I'm going to bring this in, but just noting the followups here:
|
Sorry I did the merge and forgot to add the props to it again :(
|
Follow up issue here: #59444 |
Good stuff! The best part of this effort is that all existing block themes will inherit this without having to do a thing. Anyone running a block theme can use any combination of colors or fonts, from any of their theme variations. Very nice. |
Hi 👋 , I'm sorry to be late to this conversation. I think the typography presets are not working as users would expect. Its behavior could feel unpredictable. I submitted an issue around this: #59574 |
This should have been part of #56622
I've noticed that color palettes don't show up as such if the colors-only json in the /styles directory has a "style" section in it at all. Even if the syles section only has a CSS value. I have some light CSS that is needed to change the font colors on certain background colors. For example, this doesn't show up as a color palette in v6.6. I assume the reasoning is that if a global style contains anything other than color settings, it should be retained as a global style and not generate a preset color palette that really limits what you can do to default readable text based on the background color.
Any chance that "color pairs" are in the works for theme.json and global styles? Meaning the ability to specify a background and text color at the same time? Something like this:
|
@timnicholson your assumption is correct. If the style variation contains any keys other than color it will be treated as a full style variation, not a color palette. I would be reluctant to make exceptions for specific keys because where does that end? I think the better option is to fix the issues that the CSS is trying to solve by improving Global Styles.
Not that I know of it. |
Not exactly, but you can create block style variations for group/columns blocks that apply styling to nested blocks, using whatever set of colors. |
I just noticed that WordPress v6.6 is now outputting CSS that sets the text color to the foreground color on colored backgrounds. And it uses specificity with
I was able to fix that with the additional specificity. But another issue is the two little color dots on the global theme styles are all gray no matter what the color scheme is. Does anyone know what colors it is actually pulling for those? They used to pull the first and second color in the palette (so I made the first two the "interesting" ones), but now they are all just gray. |
Do you mind opening a new issue with more details of this (visuals, active theme, etc). It's not something I am aware of.
They're using background color, heading and button/link color. |
Sorry, I jumped the gun on this. I thought it was WordPress outputting CSS for the foreground color, but it wasn't. The "bug" is actually that the Global Theme Styles preview doesn't swap out the "additional CSS" from the theme.json partials. So when I chose a style that used dark text on the light primary color, that CSS "stuck" no matter what other style I chose.
I can't get those two dots to be any color other than the foreground color. Even simply using the Block Editor Settings to change the button or link color doesn't change the color of the two dots. The only way I can get those dots to change is if I put something truly unique into the foreground color setting, like to include comments like this. This seems to override some WordPress logic that is checking to see if the foreground color in the global style json is different than that of the main theme.json. By including these comments, it wont' equal "#555555" in theme.json and thus will display the button color in both dots. Very odd bug.
|
What?
A popular feature in classic themes was to allow users to change the color palette and font families of their theme, usually the customizer. This is already possible in Global Styles, but the UI to do so is tricky to use - it takes a lot of clicks and a deep understanding of how Global Styles works. This PR abstracts that complexity away for users so they can change colors and typography details easily without knowing how Global Styles works.
These color and typography settings are extracted from the style variations, and exposed in the Global Styles UI.
Why?
This makes it possible to mix and match color and typography settings from different style variations. In the future this will also allow theme developers to offer a much wider range of color and typography variations and potentially even mix and match these variations between themes.
How?
Testing Instructions
Screenshots or screencast
There's also a demo video here: https://ben.blog/2023/12/07/global-styles-add-color-and-typography-presets/
Todo
Questions
Notes
One of the powerful things about the approach taken here is that it will work for existing themes - it doesn't require theme developers to add special settings to their themes.