-
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
Expose layout values from theme.json as css-variables #30081
Conversation
Hi @aristath Can you expand a bit on this? Why do we want to do this? What's the use case here? |
In a theme I am working on I have styles that depend on the content & wide widths. Right now I'm doing it using the custom variables in theme.json, and then setting the layout to these vars in theme.json. But that doesn't make a lot of sense and it's going in circles instead of a straight line. Now that we officially added layout it makes sense to expose these so they'll be usable in theme stylesheets directly. |
Would you mind expanding a bit more on what kind of styles these are? Do you have examples? |
Size Change: +39 B (0%) Total Size: 1.05 MB
ℹ️ View Unchanged
|
Examples: .alignleft,
.alignright {
max-width: calc(var(--wp--custom--content--default-width) / 2);
} /* default padding is ~1em when content is 74em */
body { --default-padding: calc(var(--wp--custom--content--default-width) * 0.015); }
.has-background {
padding: 0 var(--default-padding);
} There are a couple others, and of course there are other ways to do things but I wanted to make something responsive and easy to configure with as few vars as possible. Change the content width and everything else auto-adjusts. |
For the align left right, why it's not just I'm not against the change in this PR, but personally I'd not generate the CSS variable like done here because this is just going to generate at the top level and not override it if a "group" block or any container block that supports layout define its own layout, I think it makes more sense to do this in the style generation of the layout instead because this is a setting and not a style. That said, I wonder if it's really necessary to add at the moment, I mean, maybe we'll discover that the layout feature need refactoring or that your use-case are better served differently. |
Because of consistency: I wanted them to have the same max-width regardless of whether it's nested inside a standard/wide/full container...
Agreed, I was thinking of overriding the var in groups to accommodate that scenario by adding something like this: <?php if ( $content_size ) : ?>
--wp--layout--content-size: <?php echo $content_size; ?>
<?php endif; ?>
<?php if ( $wide_size ) : ?>
--wp--layout--wide-size: <?php echo $wide_size; ?>
<?php endif; ?> in the part of the code that generates the CSS: gutenberg/lib/block-supports/layout.php Lines 66 to 70 in 50cdfbe
But I wanted to get first reactions before proceeding too much with things like that 👍 |
@aristath yes, for me you can just target the container there and define the variables and it should work in all situations (no need to tie this to the custom variables generating) |
Fair point. Refactored and changed the OP description 👍 |
lib/block-supports/layout.php
Outdated
@@ -63,14 +63,18 @@ function gutenberg_render_layout_support_flag( $block_content, $block ) { | |||
if ( $content_size || $wide_size ) { | |||
ob_start(); | |||
?> | |||
<?php echo '.wp-container-' . $id; ?> { | |||
--wp--layout--content-size: <?php echo $content_size ? $content_size : $wide_size; ?>; | |||
--wp--layout--wide-size: <?php echo $wide_size ? $wide_size : $content_size; ?>; |
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.
Last remark I have is about the naming. I don't have any opinion on that, but I just want to make sure this does make sense and is consistent with what we do elsewhere cc @nosolosw
The other thing I wanted to mention is that "content size" and "wide size" are only defined and useful if the layout is "default" (which is the only layout we support right now, but I'm thinking about flex, absolute, grid layouts later. These have different kind of configs and styles). I guess this last point diminishes the need for these CSS variables because there's no guarantee that they exist.
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.
As far as naming goes, what we have is:
- presets:
--wp--preset--<preset-category>--<preset-slug>
- custom vars by the theme:
--wp--custom--<variable-name>
- styles:
--wp--style--color--link
I'm fine with whatever is consistent and clear for folks, but perhaps a suggestion would be to follow the styles naming --wp--style--layout-*
.
These styles aren't generated by the theme.json engine, though, but by the block ― so I'm not sure if it's worth making that difference clear or aligning names. One thing I've noticed is that if we do this, they become public API, which so far we've tried to avoid. I haven't been able to follow the layout work closely but by skimming quickly at this code it looks like we attach this code only if a block supports the layout and has the layout
attribute. I suppose then that there may be situations in which these aren't present (the user didn't select any layout).
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 personally still hesitant about this PR specially because the layout config can change depending on the "type" of the layout, while right now we do support just one type, I expect we'll introduce others pretty soon (horizontal flex, grid, absolute...) and in all of these these two sizes don't make sense.
What is blocking us here? |
Just adding that one of the use cases I have in my theme is to set an inner element's
I've been using I'd very much like to see this land because it'd cut back on a little custom stuff. |
Closing this one as it's no longer relevant |
Description
In #29335 we added
layout
support intheme.json
.This PR exposes these values as CSS variables (
--wp--layout--content-size
&--wp--layout--wide-size
).How has this been tested?
Tested in an FSE theme that has
layout
properly defined n its config file, and verified that bothlayout
andcustom
css-variables get properly generated.Types of changes
Exposing the CSS-variables in styles generated for containers.
Checklist: