-
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
[Try]: Style Engine: Alternative approach to rendering common layout styles from presets #39374
[Try]: Style Engine: Alternative approach to rendering common layout styles from presets #39374
Conversation
lib/block-supports/layout.php
Outdated
@@ -25,6 +25,108 @@ function gutenberg_register_layout_support( $block_type ) { | |||
} | |||
} | |||
|
|||
function gutenberg_generate_common_flow_layout_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.
Note: not all of the common styles have been added yet. In its current state, this PR is just a proof of concept that we can pass the rendered styles back up to the global styles stylesheet.
Thanks for looking into this. I'm still learning about presets and their usages so it's really helpful to see how you're using the available tools to output layout styles. 🙇 I'm thinking out loud unfiltered here so it might be all babble 😄 I'm assuming, based on a reading of the docs, that we're hooking into the presets functionality to generate the styles, and ultimately define the available layouts in the UI controls, but we're not really creating a new "preset" category per se given there are no 'values' (or 'presets') that we can configure in the theme.json (?) I might be reading it all too literally or not understanding things. I was just curious to know if we would sell layouts as a preset, or rather more as a block supports-type setting that informs the application about which layouts it is allowed to expose in the editor. Would all block-level customization styles – and there could be many depending on how many layouts we offer and their corresponding properties – be piped into the Also would theme authors want to be able to define global values for certain layout properties? For example: ....
"styles": {
"blocks": {
....
},
"elements": {
....
},
"layout": {
"flex": {
"gap": "24px",
"align": "left"
},
"stack": {
"gap": "24px"
},
"grid": {
"gap": "24px"
},
},
"spacing": {
....
},
"typography": {
....
}
}, Sorry for all the questions! I know many might not yet have answers, but I just wanted to get them down. 🍺 |
Thanks for taking a look @ramonjd, much appreciated! No apologies necessary, these are just the sorts of questions that are good to ask at this stage!
Not yet, but it does open up the possibility to do that if we wanted to explore it (which may or may not be a side-track from our goals of style consolidation). With this PR I was mostly curious to see how hard it'd be to hook into the global styles style generation and pass off the logic for constructing the styles to somewhere else (in this case the Layout block support). But the question of how much we use of the presets function is a great one — because we can pass along the preset metadata array (from the theme JSON class) and the preset data from the So, the question of how much we want to expose of layout settings, not just at the block level, but within
I was thinking that (at least at first), we'd just be focusing on outputting the "common styles" or those styles with a fixed set of options (like how alignments don't have an infinite number, maybe only a small handful of classes), and we'd then defer the highly specific values (like content width or gap value) to the rendering at the individual block level like in #38974 — and we can then make the choice between whether the classname reads in a semantic way, or if it's more obfuscated. The key part to getting this PR working in a useful way, is to still add in generating the classnames to inject at the block level when rendering the layout block support, so that the individual blocks hook up to the classnames we've generated in the global styles sheet. I thought I'd probably play with that next. But that's just what I had in mind, though, very happy to go in any other direction if anyone has strong feelings on it. And feel free to jump in on this PR if you'd like to hack around of course! I'll continue having a play with it from sometime mid next week 😄 |
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.
Thanks for getting this started! I mainly have high-level questions at this point 😄
but we're not really creating a new "preset" category per se given there are no 'values' (or 'presets') that we can configure in the theme.json (?)
This is an interesting one. We could let themes add preset values for flow width and flex gap. But are presets only valid when they are theme-configurable, or can we also have purely internal ones, in which case, potentially all the flex justification and orientation options could be presets?
Another thought, though maybe not immediately relevant: will there ever be a scenario where layout type can be picked by user? And if so I'm wondering if that might have an impact on this architecture.
lib/block-supports/layout.php
Outdated
); | ||
} | ||
|
||
function gutenberg_generate_common_flex_layout_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.
By common styles, do we mean only the styles that are shared across all flex variations (e.g. vertical orientation +justified center, or horizontal+space between), or will all the variations themselves be added in here too?
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 could be mistaken, but I understood "common" to be the "fixed" styles required for a layout to work, so common to every container that uses a particular layout, e.g., if a bunch of Group Blocks use a flex layout, then the "common style" is display: flex
for flex.
Does that sound legit?
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 understood "common" to be the "fixed" styles required for a layout to work, so common to every container that uses a particular layout, e.g., if a bunch of Group Blocks use a flex layout, then the "common style" is display: flex for flex.
Yes, that's what I was thinking of, too — common in the sense that they're styles that will be output to the global stylesheet irrespective of the blocks contained in the current post that's being rendered. (If we settle on an approach, then there'll be some a fair bit of documentation / doc comments to add!)
In that category, I was imagining we'd have things like display: flex
and possibly each of the different alignment values, since they're a fairly small fixed set of options, but probably not content width since that could be any arbitrary value. Although, perhaps we could include the parent / global level content width if it's set, since that will likely be shared by a bunch of different blocks?
But the goal I was interested in exploring was to see how far we can limit the amount of unique styles we generate.
// TODO: Before this PR merges, move this to be a part of the style engine package. | ||
// Part of the build process should be to copy the PHP file to the correct location, | ||
// similar to the loading behaviour in `blocks.php`. | ||
require __DIR__ . '/class-wp-style-engine-gutenberg.php'; |
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 move this to the style engine package, won't we need to create the same compat/wordpress-x.y
folder structure in that package as we have in lib
, to avoid clashes once the initial version of this is in Core?
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.
That's a very good question! I wasn't quite sure what the best way to handle that would be (so just left it as a TODO
for the moment). I've found the compat
folder structure and overrides for the theme JSON class a bit confusing to work with, so I was hoping we could figure out a simpler mechanism for overriding core classes somehow for the style engine.
Let me know if you think there's a particular way we should handle it! I was mostly deferring the decision until we're closer to something we'd like to land since it seemed like a bit of a rabbit hole 😅
This makes sense. Nice. I know it's probably premature to think about output, but there is some chatter over on the Layout tracking issue about whether we could look at CSS vars as well, which presets are capable of rendering. I'm mainly thinking about whichever layout values we make configurable. /* flow */
--wp--preset--layout--flow-width: 88px;
/* flex */
--wp--preset--layout--flex-wrap: nowrap;
--wp--preset--layout--flex-justification: left;
--wp--preset--layout--flex-gap: 2em;
/* grid */
--wp--preset--layout--grid-gap: 2em;
--wp--preset--layout--column: 2 / 4;
Yeah for sure! That's a neat way to look at it, given that we can control which presets we actually expose via the theme.json API, i.e., external ones like gradients vs internal ones |
Thanks for taking an early look, folks! Much appreciated.
Yes, that's a good point, I was thinking all of these could be presets (the current state of the PR is what I could hastily put together last week, but my hope was that we could have most of the configurable options rendered as presets)
Like an arbitrary layout type that we haven't already defined? If we wanted to allow custom configured layouts, I suppose we could (try to) defer configuration to the
Good question! I like the idea of CSS variables, but I was wondering if we'd wind up running into the same issues we had with gap support when it was primarily CSS variable based — that is, child blocks inheriting the layout settings of their parent due to CSS variables, even when we don't want that inheritance to happen 🤔 |
I've had a play in dec7bd8 and ac165bb with moving the common style rules out of Here's a couple of screenshots of the current state of things:
Tomorrow I might have a play with grabbing a little more from #38974 and take another look at the more unique styles that are rendered for each block, and / or those options that are a controlled set (like justification). |
Very interesting! It's pretty nice having all the rules in one, configurable spot.
Could the common style rules live in a dictionary or internal PHP/JSON/CSS/? file somewhere? Forgive me if I'm scratching up the wrong pole here, but I would have thought that the base/common styles for each layout would be part of the underlying implementation (so a theme.json extension/or a bunch of rules stored somewhere in Gutenberg) and we'd only expose configurable properties in the theme.json like gap and flexwrap. I suppose similar to the way we expose typography/spacing/border styles in blocks, and the design tools to modify them. Mainly to avoid breaking layouts altogether and to ensure, for example, that |
Thanks for taking a look! That's a great point — this PR probably exposes too much internal logic that someone could mess with and then break the layout implementation. I'll have a play with where else to put it! |
This made me think though... how extensible would we want things to be? What if theme.json authors could define their own layouts? Seems like your idea might open something like that up assuming it's palatable. 😄 |
Also a good point! A suppose it could also give theme authors a neat way to switch off auto-generated styles if they wanted a more vanilla experience, too. I might leave that bit as-is for the moment while I look into the styles that have a controlled set (justification, etc). It shouldn't be too hard to move the JSON around when it becomes clearer where it should go 😁 |
I was thinking that, once we have more layout types, we could provide the option to switch between two or types in a block. Say for a row block, there could be the option to make it a carousel. But that can also work as block variations; it's mostly down to what would make the best UX. I hadn't thought of theme-configured layouts! That would require a whole new API. But if the layout types we provide are flexible enough, themes shouldn't have to hack up their own. |
Yes, I think the ability to switch between layouts will be super powerful, particularly if we add carousel or masonry-like layouts, that'd be very cool to have for the Query Loop or Gallery blocks as a super-quick way of switching between quite different designs and layouts. |
ac165bb
to
c2780bf
Compare
Update: in 8313c31, I've had a go at adding in controlled sets of options for style generation to the JSON structure, so we can now render out some classes for justification, e.g. The naming is still up for grabs of course (I couldn't quite think of anything better than |
…e so that it's clearer which are still unique values
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.
Really cool to see some movement happening in the style engine front. Left some comments on this PR but I guess the same applies to the other one as well.
* WP_Style_Engine class | ||
* | ||
* @package Gutenberg | ||
*/ |
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'll focus my review on this file, as If I'm not wrong, this is the "style engine" package equivalent. Well I guess my first question is, should we try to move this file to the package (like we do for "parser", and "block-library")?
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.
should we try to move this file to the package (like we do for "parser", and "block-library")
Good question! I suppose it depends on what will be easiest for when the feature needs to be merged into core. I see Ramon asked a similar question over on: #39446 (comment) — given that PR will be in better shape to land before this one, it might be better for us to explore where the file goes in that PR. For the moment, I mostly just put it here for convenience while hacking around!
* @since 5.5.0 | ||
* @var array | ||
*/ | ||
private $registered_styles = array(); |
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.
Does the style engine need to be stateful? The JS version is stateless, so I was wondering if we should have the same signature/API?
I guess the question here is what is $resitered_styles
for?
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.
(Expanded on my questioning here on my next comment)
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.
Does the style engine need to be stateful?
It's a great question, and I think the answer is most likely a "yes".
I'll give a more thorough answer to the other comment, but there are two main reasons to have state:
- To consolidate and de-dupe styles.
- To enable outputting a single set of styles to a single
style
tag or stylesheet. (Instead of appending an additional style tag when each block is rendered).
In my experimenting so far, there doesn't appear to be a way around collecting the styles we would like to later output without having some kind of state.
* | ||
* @return string The class name for the added style. | ||
*/ | ||
public function add_style( $key, $options ) { |
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.
Ok, so If I'm understanding properly, the style engine here has a different meaning than the style engine package in JS.
The style engine package in JS is responsible for "style object -> css rules" transformation (potentially also css rules -> css string) while the current php style engine is more for "css rules -> css string". Please correct me if I'm wrong.
I kind of feel like the first step is actually the most important for the style engine work (style object -> css rule) and the second step is an optimization step (array of css rules -> css string).
That said, that's fine, if we're focusing for the second part of the style engine, I'm wondering why are we building the API like that:
php
$engine->add_style( $rule );
$engine->add_style( $rule );
$engine->add_style( $rule );
$css = $engine-> output_styles();
instead of just
$css = engine_output_styles( $rules );
the latter being stateless and also more aligned with what I think will be the output of the first step of the style engine (an array of rules).
I guess the proposed API is motivated by the fact that the current block supports are "discrete" php files but personally I feel like they shouldn't be discrete, it's just the way they were implemented originally. I guess we can try to discuss this.
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.
Thanks for checking this out!
I'll let @andrewserong elaborate with specific reference to this PR, but I have some general thoughts:
I kind of feel like the first step is actually the most important for the style engine work (style object -> css rule) and the second step is an optimization step (array of css rules -> css string).
Agree. The first step is happening over at:
#39446 👍
I see this following the stateless approach you mention.
This PR (and #38974) is part of our explorations into what's possible and what we (think we) want to see down the track as well (CSS consolidation, optimisation and a better developer experience).
I think we also wanted to noncommittally test some of the hypotheses/feature requests that have been floating around, e.g.,
- obfuscation
- consisten css classname hooks
- deduping
Layout was a good starting point for this given the amount of CSS we generate on the frontend and rule duplication.
I guess the question here is what is $resitered_styles for?
I believe it stems from the long-term idea that we could render all CSS in one style block on the frontend (instead of many), and, in doing so, leave open the possibility for pre-render processing/filtering of CSS rules.
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.
Thanks for the questions @youknowriad! I'll answer slightly out of order to hopefully explain some of my thinking 🙂
At the outset, I do want to flag that in the current state, this PR is weird and experimental! 😁 My approach at the moment is to stuff this PR full of ideas / hack things in there to get them working, and then gradually pull things out into something neater.
Just echoing what @ramonjd wrote above, but the goal of this PR was to take a bit of a further out look to see what kind of end point we'll need to reach if we want to achieve some of the things we're hoping to get out of the style engine work. In Ramon's PR (#39446), the stateless / closer to the JS package version is being explored, and that'll likely be in good shape to merge much sooner than this PR.
I kind of feel like the first step is actually the most important for the style engine work (style object -> css rule) and the second step is an optimization step (array of css rules -> css string).
Absolutely. But I think that optimisation step could be harder to implement if we don't design for it — I'm keen for us to improve the style output (primarily de-duping and consolidating to a single stylesheet), and we get quite a bit for free if we add in the internal state and separate out the two steps (adding styles and output).
I think being able to separate generating and outputting styles into separate steps via some internal state will probably allow us some good flexibility for doing things like:
- De-duping styles
- Sharing classnames or styles across functions / at different execution times
- Being able to access generated styles separately — for example, if we want to explore at some point making generated styles available via an API endpoint, having easy access to that state will make those explorations much easier
While those ideas sound very "optimise-ey" — i.e. things we'd usually think to do in later steps, and not do prematurely — here, we've got some concrete optimisations that we know we need to make and solve for, so I think it's worth it to dig in a bit to see what might look viable.
I guess the proposed API is motivated by the fact that the current block supports are "discrete" php files but personally I feel like they shouldn't be discrete, it's just the way they were implemented originally. I guess we can try to discuss this.
It's less motivated by the separate PHP files, and more motivated by styles being output one block at a time as each block is rendered, resulting in one style
tag per rendered block (that uses the container
id approach). We currently abstract that away in a small way in gutenberg_enqueue_block_support_styles — but my hunch at the moment is that we'll benefit from having some more smarts around this rather than relying on the wp_footer
and wp_head
hooks directly.
However, this is all still very much experimental! I'm more than happy for us to see how far we can get with the stateless approach in the shorter-term — my main interest right now is to ensure that if we do want to add in the stateful approach it's easy enough for us to migrate it in there to address optimisation, rather than having to rewrite everything! So, at least from my perspective, the current approach has pointed toward using a class for the style engine so that we can encapsulate (and namespace) our logic, and make it easy to add in the internal state if / when we need it.
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.
Thanks that make sense.
It's less motivated by the separate PHP files, and more motivated by styles being output one block at a time as each block is rendered, resulting in one style tag per rendered block (that uses the container id approach).
That makes sense but personally what I don't like is the singleton because it's very hard to reason about depending on where you are (regular php template, api...). I'm just nitpicking at this point but I'd have a small preference for something more explicit.
Basically, what I don't like about the singleton approach (and I guess all regular WP style php) is that it's all dependent on order of execution of things, actions... It's not really clear when are we allowed to add styles to the instance, from where and who can do it. So we end up with hidden bugs, for instance what happens if I add a style to the singleton after the generate
function has been called?
My "weird" mind prefers something more dumb :P
function render_blocks( $blocks ) {
$markup = '';
$rules = array();
foreach( $blocks as $block ) {
list( $css_rules, $block_markup ) = render_block( $block );
$markup .= $block_markup;
$rules = array_concat( $rules, $css_rules );
}
return [ $rules, $markup ];
}
list( $rules, $markup ) = render_blocks( $my_block_list );
$css = generate_optimized_stylesheet( $rules );
I guess it might not be easy to implement because it kinds of touches the fundamental of the existing do_blocks
but anyway it's worth sharing just in case 🤷
we can also imagine that later we could also return JS in addition to "css_rules" and "markup" if we want to do JS optimizations.
list( $css_rules, $block_markup, $scripts ) = render_block( $block );
Wanted to clarify that there's no blockers from me here, I'm just basically sharing my mental model in case it helps in any way.
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'm just basically sharing my mental model in case it helps in any way.
Thank you, it's always very much appreciated! 😊 That code has a more JS-ey flavour to it that immediately feels more at home to me, too.
You make great points about the challenges with singletons, actions, and the order of execution. I hadn't even thought of seeing if we could mess with the render_block
API 😄 — while changing the return type there could open up a can of worms, this is some very helpful nudging for us to try to keep the API we design explicit and hopefully simple to work with. I'll continue having a play!
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.
Basically, what I don't like about the singleton approach (and I guess all regular WP style php) is that it's all dependent on order of execution of things, actions... It's not really clear when are we allowed to add styles to the instance, from where and who can do it. So we end up with hidden bugs, for instance what happens if I add a style to the singleton after the generate function has been called?
Hmm, valid point, and the example is very helpful!
I suppose we haven't really decided where we'd dump the contents of every registered style 😄 It's a good excuse to dig into render_block
too to test the idea.
Very excited about the potential direction of this PR! I want to flag that it is extremely aligned with a lot of the suggestions in #38998 which discusses a number of the benefits of this approach for theme portability, CSS customization, and plugin-theme interoperability. Specifically, that proposal hopes to:
@andronocean had a fabulous summary of the benefits and details of how a system like this should work that I quoted at length. |
Thanks for the feedback everyone! I'm pleased to see folks are interested in the direction this is taking and that it looks like it could address some of the issues raised in the CSS standardisation discussions. There's a couple of other alternatives being explored, too (e.g. #39708), but it sounds like going in the direction of presets for common styles is looking promising 👍 In the immediate term, with the WordPress 6.0 timeline approaching, I'm going to pause working on this particular exploration for a little while, to focus on shorter-term layout issues and bug fixes / enhancements like those in the prerequisites section of the layout options tracking issue, that we can hopefully land in time for 6.0. |
Closing out this experiment now that #40875 has landed which progresses a consolidated class-based approach to common layout styles (and the style engine is progressing nicely toward style optimisation, too). |
What?
🚧 🚧 🚧 🚧 🚧 WIP: This is a messy exploratory PR that is highly likely to change 🚧 🚧 🚧 🚧 🚧
This is a very early draft exploration alternative to #38974 to look at potentially trying to generate common layout styles via presets in
theme.json
.The main objective is to hook into the
theme.json
style generation for invoking generating styles for layouts.Why?
Based on discussion in #38974, we'd like to look into a presets approach for generating common layout styles, that individual blocks can then link to via classname.
How?
Currently it adds an additional preset metadata item for layouts, and then iterates over those layout types — there's an escape hatch from the usual global styles generation that then goes to call a function from the Layout block support to generate the styles. This then uses a similar mechanism as to #38974 to generate the styling rules, and then passes it back to the theme JSON class, so that those styling rules can be included in the global stylesheet.
This is a very naive approach at the moment, and it is not close to a working example.
Testing Instructions
Screenshots or screencast
Styles included in the global styles sheet: