Add initial version of the style engine #37978
Conversation
youknowriad
requested review from
oandregal,
jorgefilipecosta,
andrewserong and
aaronrobertshaw
|
Size Change: +506 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
|
Nice work, thanks for getting this started @youknowriad! Lots of great decisions here, I like the simplicity of the API that gets exposed ( This might be premature, but one thing I'm curious about, for getting parity between frontend and backend styles, and being able to one-day resolve #35376 is how we might go about getting / giving access to the |
There was a problem hiding this comment.
Amazing start so far @youknowriad 🙇
Lots of great decisions here, I like the simplicity of the API that gets exposed (generate and getCSSrules).
Exactly. The API looks great and gets another +1 from me.
I think one issue with the current block supports, global styles and theme.json has been that the documentation has been a bit lacking in terms of providing devs with easily consumed real-world examples. In many cases, it relies on people reading through the code to fully understand how it all comes together before they can then apply that knowledge to their use case.
Given the discussions to date around the styles engine effectively give us a checklist of features we'd like it to have. Would it be prudent to add usage examples as these features and capabilities get added? I appreciate it's very early days but could save a lot of "support" in the future.
| } | ||
| } | ||
| } ); | ||
|
|
||
| // The goal is to move everything to server side generated engine styles | ||
| // This is temporary as we absorb more and more styles into the engine. | ||
| const extraRules = getCSSRules( styles, 'self' ); |
There was a problem hiding this comment.
Will getCSSRules need to return React-safe style camel case props as well, e.g., { style: { paddingTop: '19px', ... } }
I'm seeing mixed output:
I know the focus is on rendering styles to the page, so apologies if I'm jumping the 🔫
There was a problem hiding this comment.
Indeed, I guess I didn't pay enough attention here :).
For me getCSSRules was supposed to be an internal API (not exposed) but I had to find something to make the current style hook and theme.json style renderer continue to work with the padding moved to the style engine. So I just used what I had. I'll need to check where the mixing happens and just pick one for now.
There was a problem hiding this comment.
Interesting problem here. I had a play with a couple of different approaches and settled on switching getCSSRules to return the camelCase keys (since it's an array of JS objects, it felt slightly more natural to me for it to be using the JS style attribute convention), and then get generate to be responsible for converting it to kebab-case when outputting the CSS styles. Updated in this commit: a756e44
That seemed to fix it for me, though it does introduce importing another couple of lodash functions. I imagine we might want to see if we can get rid of the lodash dependency if we're expecting the style engine to also be run in standalone frontend environments outside of Gutenberg, but for the moment at least, it made it quicker to get the fix in.
Happy to change the behaviour here if folks have a preference for the shape of the attributes!
|
Thanks for starting this and giving us a sneak preview of your idea ❤️ Very curious to see how this develops, in particular, how/whether could ultimately leverage a backend implementation to register, dedupe and bundle CSS definitions in one
Probably a bit down the track! |
|
I'm just adding myself as an assignee. For anyone else reading on, I've offered to take over this PR from Riad, so will give it a rebase and work through some of the things we encountered when we first took a look at. |
andrewserong
force-pushed
the
add/style-engine
branch
from
88a2eb6
to
4452f09
Compare
| @@ -99,6 +99,7 @@ export const __EXPERIMENTAL_STYLE_PROPERTY = { | |||
| paddingBottom: 'bottom', | |||
| paddingLeft: 'left', | |||
| }, | |||
| useEngine: true, | |||
| }, | |||
There was a problem hiding this comment.
Ideally at the end of the style engine implementation, this "constant" might be removed entirely if we did our work properly :)
| @@ -146,7 +147,7 @@ function flattenTree( input = {}, prefix, token ) { | |||
| * @return {Array} An array of style declarations. | |||
| */ | |||
| function getStylesDeclarations( blockStyles = {} ) { | |||
| return reduce( | |||
| const output = reduce( | |||
| STYLE_PROPERTY, | |||
| ( declarations, { value, properties }, key ) => { | |||
There was a problem hiding this comment.
I think we should probably ignore the properties with useEngine: true here kind of like https://github.com/WordPress/gutenberg/pull/37978/files#diff-292053c9ec98ebdd73943767587e8093315ad3a2bf01ca3b5352efc48326becdR89
There was a problem hiding this comment.
Good catch! I've added useEngine to the check that bails early in the loop in 31981a1 — I've also updated the output slightly to match the change to getCSSRules discussed here.
|
I've made a couple of updates to the PR and the unit tests are passing now, with the change in |
andrewserong
added
[Type] New API
andrewserong
requested review from
ajitbohra,
ellatrix and
gziolo
as code owners
I don't know if you are referring to me, I guess not. But as we talked about the implications of this PR for frontend hydration, I'll add my review here: It looks great to me!! 👏 That's it 😄 |
| @@ -0,0 +1,45 @@ | |||
| export type Box = { | |||
There was a problem hiding this comment.
Have you considered using the React.CSSProperties types for most types in this file?
e.g
type Props = {
backgroundColor: CSSProperties[ 'backgroundColor' ];
}There was a problem hiding this comment.
Good question, we can probably move most of them over to CSSProperties (e.g. fontWeight, fontStyle, etc). For the Box type, since it's used in both margin and padding, it might need to be a bit more flexible than targeting a single CSS property. What do you think would work for that type?
There was a problem hiding this comment.
Mhh, not sure about that. The main thing is that the idea of Box (an object with top right bottom and left props) doesn't really translate to CSS.
If we need to maintain the structure of these types, we could still use CSSProperties where we're using string, and leave the rest unchanged:
export type Box = {
top?: CSSProperties[ 'top' ];
right?: CSSProperties[ 'right' ];
bottom?: CSSProperties[ 'bottom' ];
left?: CSSProperties[ 'left' ];
};
export interface Style {
spacing?: {
margin?: CSSProperties[ 'margin' ] | Box;
padding?: CSSProperties[ 'padding' ] | Box;
};
// ...
};WDYT?
There was a problem hiding this comment.
Oh, I quite like that, and it lets us keep the Box abstraction. It isn't quite right because it's referring to different properties, but the underlying type should be much the same, and it still serves the purpose of better communicating the desired type. I've updated in f0cbeff, and we can continue to tweak the types in follow-ups, too 🙂
There was a problem hiding this comment.
Thank you! I think it's already much better.
It isn't quite right because it's referring to different properties, but the underlying type should be much the same,
Out of curiosity, what did you mean with "referring to different properties" ?
There was a problem hiding this comment.
what did you mean with "referring to different properties" ?
Apologies, I could have been clearer there! What I meant is that CSSProperties[ 'top' ] refers to the top property and not for example padding-top — but since they both accept pretty much the same actual values, it doesn't matter too much that the type isn't precise. I think CSSProperty[ 'top' ] is preferable to a more complex type like CSSProperty[ 'top' ] | CSSProperty[ 'paddingTop ' ] | CSSProperty[ 'marginTop' ] which could get a bit unwieldy, even if it's technically more accurate.
But, happy to explore in follow-ups, of course!
There was a problem hiding this comment.
Proposed a potential follow-up in #38894
…s object instead of selector string
andrewserong
force-pushed
the
add/style-engine
branch
from
b7150e2
to
f0cbeff
Compare
|
I've implemented the latest couple of bits of feedback and given this a rebase. It's still technically the weekend in other parts of the world, so I'll leave this overnight my time and merge in tomorrow if there are no objections! |
There was a problem hiding this comment.
I think this is good to land now, and there've been no objections in proceeding further, so I'll give it the ✅😀
Thanks again for all the feedback and testing, folks! Let's continue discussions on the tracking issue (#38167) and related discussions.
This is an exploration to try to find a path towards what's being discussed in #37495
My main goal here is not to refactor everything but to try to bootstrap a way for us to iterate without breaking changes or anything and build that engine iteratively until we can pull the plug entirely.
This is very early but worth sharing to start concrete discussions
Features include
packages/style-enginepackage.generatefunction that takes a style object and an options object (which allows for defining a selector), and returns rendered CSS.getCSSRulesfunction is also exposed, that returns the rendered styles in an array of objects that provides aselector, akeythat matches the CSS property used, and avalue.How to test this change