Style Engine: add typography and color to frontend #40665
Conversation
ramonjd
added
[Status] In Progress
|
Size Change: +437 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
ramonjd
force-pushed
the
update/style-engine-frontend-color-typography
branch
from
ba35a2f to
be71be4
Compare
ramonjd
force-pushed
the
update/style-engine-frontend-color-typography
branch
from
85065b1 to
978da19
Compare
packages/style-engine/src/index.ts
Outdated
| * | ||
| * @return string[] An array of classnames. | ||
| */ | ||
| export function getClassnames( style: Style ): string[] { |
There was a problem hiding this comment.
I think we could probably think about/ship this functionality separately.
Just adding it here to evaluate the concept.
There was a problem hiding this comment.
I like the concept! I think I'd lean toward including it in this PR, it's looking like a good direction to me.
There was a problem hiding this comment.
Who's responsible for providing the styles for these classnames, while I think having a function that return "state" classnames is fine, I might think that these classnames shouldn't be necessary to style things properly and if we can have all the logic in generate function it's a bit better.
There was a problem hiding this comment.
I guess we can't replace classnames with CSS variables yet in generate because that would require block deprecations for now. It might be fine to keep these out for now with a caviat that we may change these APIs later (to fully fulfill the style engine's potential :P)
Also, I don't see this function used in the "style" hook, should we replace the custom logic there with this function (like we did with the inline styles)?
For Global Styles generation though, it seems that we'll have to implement the CSS variable based styles in generate anyway right?
There was a problem hiding this comment.
Who's responsible for providing the styles for these classnames,
In the post editor, the styles linked to the has-* classnames are ultimately pulled from global settings/presents (generated by class-wp-theme-json-gutenberg.php).
An inline JS script hydrates the editor with settings. The styles are passed down from and transformed/output in .
Then in hooks/color.js the classnames for the block wrapper are compiled and added to the block props.
Is that what you meant by "responsible"?
The last step is what I had in mind for the style engine to take over initially, only because we're doing it in the backend. I was playing around with generating the has-* classnames in hooks/color.js via the style engine this morning. It works okay, but I get the feeling I'm building "around" existing logic, so not sure yet.
while I think having a function that return "state" classnames is fine, I might think that these classnames shouldn't be necessary to style things properly and if we can have all the logic in generate function it's a bit better.
Now I'm writing about it, it might be a better approach to have the style engine generate the CSS styles at the transformed/output stage mentioned above and forget about block classnames for now.
I guess we can't replace classnames with CSS variables yet in generate because that would require block deprecations for now.
Yeah, I did have a shot at putting the CSS vars inline... and it worked 🎉 but the console exploded in my face 😄
Also, I don't see this function used in the "style" hook, should we replace the custom logic there with this function (like we did with the inline styles)?
I tested it out this morning, and have things half-working. There are some inexplicable fixture-related errors, but I'll push anyway so can eyeball the code.
For Global Styles generation though, it seems that we'll have to implement the CSS variable based styles in generate anyway right?
Yeah for sure. That will be its own challenge. It makes me think that I should leave the block classname alone for now and rather devote the time into centralizing style generation.
There was a problem hiding this comment.
It makes me think that I should leave the block classname alone for now and rather devote the time into centralizing style generation.
I've pushed my work so far in 393fac4 so we can look.
There was a problem hiding this comment.
Yeah, I did have a shot at putting the CSS vars inline... and it worked 🎉 but the console exploded in my face 😄
But isn't this the right thing to do anyway. It will solve both "block style generation" and "global styles generation" at the same time with the same style engine.
I agree that we need to find a way to either:
1- Don't use the style engine yet for colors and such in the hooks/color.js for now and keep the existing logic there
2- Have some kind of flag per block to say: this block is ready for full style engine style generation which "for me" means:
- do not serialize inline styles in the saved content
- rely on the JS style engine for the "edit" function (injected somewhere in hooks/color)
- rely on the PHP style engine for the frontend.
Maybe it's too soon for 2, I don't know.
393fac4 is fine if we really want to keep getClassnames to centralize the "classname" generation for blocks based on styles, but for me, this is not really the style engine, or more precisely, not the main purpose of the style engine.
I hope that makes a bit of sense :)
There was a problem hiding this comment.
Don't use the style engine yet for colors and such in the hooks/color.js for now and keep the existing logic there
Yeah, what this PR is attempting might be too much too soon. There's probably more value in trying to get style/css generation right across blocks/global styles.
rely on the PHP style engine for the frontend.
It would be fabulous for the PHP to take care of all frontend styles. Not sure about inline CSS saved in the editor though... We could maybe start over at layout.php for now.
want to keep getClassnames to centralize the "classname" generation for blocks based on styles, but for me, this is not really the style engine, or more precisely, not the main purpose of the style engine.
I get you. My idea was to take as much decision making about classnames and styles away from the editor as possible. Classname generation might be a bit premature I'd admit that.
I'd be fine with skipping this functionality in the JS in this PR. Maybe even revisiting the backend functionality too.
I'm still holding onto the hope that it'll all be clearer after we experiment with things. 🤞 Am I crazy? 😄
I hope that makes a bit of sense :)
Yes, it's given us a lot to think about, thanks a lot!
There was a problem hiding this comment.
Maybe it's too soon for 2, I don't know.
Thanks for laying out a bit of how that could work — I think that sounds like something to pursue in a separate PR (perhaps once we've got all the block supports implemented server-side?) but I really like the idea of a per-block flag to opt-in for the style engine to render out the styles server-side on the site frontend. We could also use that flag to conditionally ignore style validation errors as a gentle way of gradually introducing the work from #37942 (your PR to ignore style validation errors) so we can avoid issues with deprecations.
There was a problem hiding this comment.
Thanks for clarifying 👍 I'm confident this is going in the right direction ❤️
|
Will fix the failing JS tests at some point 😇 |
There was a problem hiding this comment.
Nice work @ramonjd! This is looking like a really solid direction to me, there's some good parallels with the server-side implementation and I think it's starting to really highlight the value of us having the style engine as a separate package with common patterns for how each of the rules are implemented.
I've left a bunch of comments (mostly just some nits), but it's looking like a good path forward to me. Let me know if there's anything in particular you'd like me to test in further detail!
packages/edit-site/src/components/global-styles/use-global-styles-output.js
Show resolved
Hide resolved
packages/style-engine/src/index.ts
Outdated
| * | ||
| * @return string[] An array of classnames. | ||
| */ | ||
| export function getClassnames( style: Style ): string[] { |
There was a problem hiding this comment.
I like the concept! I think I'd lean toward including it in this PR, it's looking like a good direction to me.
test/integration/fixtures/blocks/core__pullquote__custom-colors.serialized.html
Outdated
Show resolved
Hide resolved
|
Just to note that this PR breaks link color style output. The presets aren't parsed. So a block like this, <!-- wp:paragraph {"style":{"elements":{"link":{"color":{"text":"var:preset|color|vivid-red"}}}}} -->
<p class="has-link-color"><a href="https://test.com,">Test</a></p>
<!-- /wp:paragraph -->The output is: .editor-styles-wrapper .wp-elements-3 a{
color: var:preset|color|vivid-red; /* Should be var(--wp--preset--color--vivid-red)*/
}Looking at this now 👀 |
ramonjd
force-pushed
the
update/style-engine-frontend-color-typography
branch
from
95ab733 to
d051c9f
Compare
@andrewserong I've removed support for The link text color value needs to be converted from This is fine in itself however the style objects for links and text colors, at least the ones passed to At this line of The full object would be Since the link text color and regular text color objects are the same at this point, the style engine doesn't know that it should parse One possibility is to parse all style values for We can do that and it will work (I tested it). However if we do that, we might need to rethink the way we generate classnames, given that the current "rule" for background colors and other styles is use Long story short, the style engine would need to know when to return a CSS var and when not to for a particular style value. Given it's the style hook that's giving the style object a haircut, I think a neater way would be to pass the entire style object, including the This means we might need to concentrate and/or refactor hooks/style.js, which would entail a bit of faffing around in another PR. What do you reckon? Sorry if that doesn't make sense. I've been thinking about it for too long. Can you think of a way of dealing with |
I've done that in 2cf8698 The consequence is that we'll parse all style values for Maybe it's a better way to start off and we can deal with classname + element stuff later, the backend notwithstanding 🤷 |
For the other colors that might reference a variable (e.g.
That seems like the most straightforward way to handle it to me, since it looks like that's the current behaviour in Apologies, I very well might be missing quite a bit of context here! |
Thanks for trying to decipher my gibberish @andrewserong I ended up doing this anyway. What I was trying to express with all the Github comment anguish was this: in the backend So 'color' => array(
'background' => 'var:preset|color|burned-toast'
)Tells the style engine to return See #40332 (comment) for some context. This logic doesn't exist in the frontend right now. Maybe it doesn't have to, and all my hand-wringing is for nothing. Besides, we don't really have to deal with it yet since we're not using the getClassNames method. My guess is that it will come out in the wash as we progress. |
Ah, I see now! Thanks for the explanation, this is the line where you're skipping generating the CSS rule if it starts with
Yes, it's a little tricky to work out which way we should go here. I think I'd lean toward seeing how far we can get with the current approach, even if it creates a slight gap between frontend and backend, and then revisit when we explicitly go to incorporate the Elements block support — that'll wind up involving dealing with non-inline styles, too, so it becomes a bit of a rabbithole to figure out. One of the other differences we still have between front and backend is that in the frontend code we're also calling the style engine from the global styles output, which we haven't started doing in the backend yet. So probably good to see if we can handball some of these questions to follow-up PRs where we can dig in, in isolation? |
Agree. Once again, thanks for helping out with the thinking here. 🙇 |
ramonjd
force-pushed
the
update/style-engine-frontend-color-typography
branch
from
cb36e10 to
c726f9b
Compare
Updating tests Refactor styles directory
Fixing camel case style prop name
… It's my PR and I'll fail if I want to. Preferencing `uniq` for new Set() spread Updating tests.
… of the inline styles rules. Nothing major.
Updated inline docs Making the generateRule signature the same as generateBoxRules
Co-authored-by: Andrew Serong <[email protected]>
Removing styles color text support because we haven't yet come up a with a way to deal with link color styles for elements. We need to reconcile the way we generate text colors with link css var colors and the logic in hooks/style.js Add constants Updated inline docs Making the generateRule signature the same as generateBoxRules
…oesn't need to pollute the codebase.
…d return CSS vars all the time.
…the generic class name if required. Regenerating fixtures due to the re-ordering of color classnames.
…at in another PR.
ramonjd
force-pushed
the
update/style-engine-frontend-color-typography
branch
from
07bafe3 to
41dbe14
Compare
There was a problem hiding this comment.
Thanks for tidying this up @ramonjd, I think this is looking very close to merge to me.
Tested with the following markup (and making lots of tweaks in the editor):
Block markup
<!-- wp:group {"style":{"color":{"gradient":"linear-gradient(135deg,rgb(252,185,0) 18%,rgb(255,105,0) 100%)"}}} -->
<div class="wp-block-group has-background" style="background:linear-gradient(135deg,rgb(252,185,0) 18%,rgb(255,105,0) 100%)"><!-- wp:heading {"fontFamily":"system-font"} -->
<h2 class="has-system-font-font-family">Heading</h2>
<!-- /wp:heading -->
<!-- wp:paragraph {"style":{"typography":{"fontSize":"0.53rem"}}} -->
<p style="font-size:0.53rem">A paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph {"style":{"typography":{"fontStyle":"italic","fontWeight":"800","textTransform":"capitalize","lineHeight":"5","letterSpacing":"13px"}},"fontSize":"medium"} -->
<p class="has-medium-font-size" style="font-style:italic;font-weight:800;letter-spacing:13px;line-height:5;text-transform:capitalize">This is a paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph {"style":{"elements":{"link":{"color":{"text":"#f66d6d"}}},"color":{"background":"#6d0e0e","text":"#f0f0f0"}},"fontSize":"large"} -->
<p class="has-text-color has-background has-link-color has-large-font-size" style="color:#f0f0f0;background-color:#6d0e0e">Another paragraph with <a href="http://www.imdb.com">a link</a>.</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p></p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->
<!-- wp:read-more {"content":"Read more","style":{"typography":{"textDecoration":"underline"}}} /-->
✅ Color is output correctly
✅ Background color is output correctly
✅ Font size is output correctly
❓ Font style is output correctly, but it looks like we need to add the useEngine flag to it (tests well manually if I change that flag)
✅ Font weight is output correctly
✅ Line height is output correctly
✅ Text decoration is output correctly
✅ Text transform is output correctly
✅ Letter spacing is output correctly
❓ Font family doesn't appear to be opted-in — was that intentional? I assume so, as it's currently implemented via class names in the editor for e.g. Heading block? I left a comment to discuss further, I ran out of time today to look it up myself 😅
Happy to do more testing / a final review tomorrow. Thanks again! 🙇
Remove the handler for fontFamily to reflect the current situation in the editor whereby no custom fontFamily style values are expected. Removed unused function `getSlugFromPreset` Added tests for elements.link.color.text preset style values.
There was a problem hiding this comment.
Thanks for making the updates @ramonjd! I've just re-tested and that's looking good now.
✅ Font style is output correctly
✅ Font family is no longer included
✅ Included utils functions are used, and no strays left behind, and logic in getCSSVarFromStyleValue matches that used in computeStyleValue in hooks/style.js 👍
This LGTM now! 🎉
What?
Experimental PR to see if and how we can port backend features added in #40332 to the frontend in relation to color and typography
Why?
To unify the frontend and backend.
It might not work out since styles and classnames aren't added in the same callback 🤷
How?
Adding color and typography CSS rules to the existing ruleset (spacing).
The addition of a new methodgetClassnamesto generate block classnames based on preset attribute values passed to the style engine something similar tovar:preset|color|burnedToastTesting Instructions
Insert a group block into the post editor and tweak the typography and color styles.
Here's some test HTML
Block code
Check that the block appears as it should in the editor and the frontend.
Unit tests
More to come