Block gap: Only render CSS variable if corresponding theme setting is enabled#35209
Conversation
|
Note: this still needs to have tests added, but I thought I'd get the PR up as a proof of concept. |
| $value = self::get_property_value( $styles, $value_path ); | ||
|
|
||
| if ( in_array( $css_property, self::PROTECTED_PROPERTIES ) ) { | ||
| if ( ! _wp_array_get( $settings, $value_path, false ) ) { |
There was a problem hiding this comment.
I think we'll want to null check this in order to be consistent with #34491, which makes the block gap styles explicitly opt-out rather than opt-in; according to that PR setting the blockGap setting to false should turn off the block support UI but not the global gap styles.
There was a problem hiding this comment.
Of course, the drawback to explicitly checking for null is that we'll end up with styling regressions on blocks themes that don't opt in to the block support, but do opt in to the global block gap styling (ie blockGap: false or omitting the setting entirely).
I think that just might be a limitation of the unified variable approach 🤔
|
Thanks so much for this, @andrewserong! Great find -- what a tricky one to track down. This is looking really good so far for me! I tested on Twenty Twenty One/Seedlet and confirmed that the variable is no longer generated, and on TT1-blocks I confirmed that it's added correctly. I just left one comment about blocks themes that set I like this approach, although I wonder if needing to explicitly check for |
3cdcc6f to
afb35ef
Compare
|
Thanks for testing and for the suggestions @stacimc! I've updated this PR to use an explicit check for |
|
@youknowriad I've just added you as a reviewer since you've done a lot of the work on the layout and gap support. What do you think of this approach for ensuring that we don't render the |
| * These properties are only rendered if a corresponding setting in the | ||
| * `PROPERTIES_METADATA` path enables it via a value other than `null`. | ||
| */ | ||
| const PROTECTED_PROPERTIES = array( '--wp--style--block-gap' ); |
There was a problem hiding this comment.
A small thing: Should we have something like 'spacing.blockGap' => 'spacing.blockGap' instead. Meaning (theme.json style property mapping to its corresponding theme.json setting key). The current logic relies on the output instead of the input and assumes the setting key is the same as the style key (which is of course true now).
…to setting key mapping
|
Thanks for reviewing and for the suggestion @youknowriad! That feels better to me explicitly describing the mapping so that we can support a variance between the style key and settings key if we need to for future properties 👍 I went for something close to what you suggested, using Since it's a fairly minor change, I'll go ahead and merge it in once tests pass, but happy to do any further changes in follow-ups if need be! |
Description
Fixes #35197
In #35197 it's described that rendering the
--wp--style--block-gapCSS variable globally when the block gap setting hasn't been explicitly opted in-to, could result in styling regressions on classic themes that useadd_theme_support( 'experimental-link-color' );if we're to proceed with updating Button block styling to use the new CSS variable.In this PR, we're exploring skipping rendering the CSS variable if the corresponding setting in
theme.jsonis set tonull. The mechanism is to add a list of protected properties, and for these particular properties, only render them if their corresponding setting intheme.jsonis not null. This behaviour is consistent with the approach in #34491 to opt out of generating block gap styling. The extension here is to also skip rendering the CSS variable itself.This is just one particular approach to solving the issue, so please let me know if you think it's a bad idea, or if you can think of a better way to handle it.
How has this been tested?
With TwentyTwentyOne theme enabled, open up the post editor and inspect element, and find the set of four or so
styleelements that sit underneath the visual editor component. Before this PR is applied, you should see that the global--wp--style--block-gapCSS variable is rendered.With this PR applied, it should no longer render that variable with that theme active.
With TT1-Blocks active, and
settings.spacing.blockGapenabled in thetheme.jsonfile, then the CSS variable should be rendered.With TT1-Blocks active, and
settings.spacing.blockGapset tonullin thetheme.jsonfile, then the CSS variable should not be rendered.Screenshots
TT1-Blocks (should remain unaffected when the
settings.spacing.blockGapsetting is enabled intheme.json:Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).