Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Try using variable for root padding value #39926

Closed
wants to merge 25 commits into from
Closed

Try using variable for root padding value #39926

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
d147da4
Try using variable for root padding value
tellthemachines Mar 31, 2022
24de826
Root vars shouldn't apply to block global styles
tellthemachines Apr 1, 2022
2b07b15
Fix issue with shorthand padding on server side.
tellthemachines Apr 5, 2022
59d23c4
Temp fix for file path error
tellthemachines Apr 5, 2022
e2be9f3
Fix site editor top padding not updating
tellthemachines Apr 6, 2022
9f36297
Support shorthand properties in site editor.
tellthemachines Apr 7, 2022
88035d5
Fix full widths in the post editor
tellthemachines Apr 7, 2022
34f7205
Check selector inside function.
tellthemachines Apr 7, 2022
1d2e630
Fix kebab-casing of CSS variables.
tellthemachines Apr 7, 2022
5f9da6d
Fix borked merge conflict solving
tellthemachines Apr 8, 2022
e68ae13
Move post editor full width styles to edit-post package.
tellthemachines Apr 8, 2022
688376a
Set default padding value to 0.
tellthemachines Apr 8, 2022
a9b8fc7
Update test string.
tellthemachines Apr 8, 2022
3ab23bf
Fix PHP unit tests
tellthemachines Apr 8, 2022
c21cc1b
Fix PHP lint
tellthemachines Apr 8, 2022
4b02d65
Fix failing PHP tests.
tellthemachines Apr 11, 2022
54e0e7c
Use block gap variable as default root padding value.
tellthemachines Apr 11, 2022
e69a08c
Fix linting errors.
tellthemachines Apr 11, 2022
43c84a5
Add opt-in setting via package.json
tellthemachines Apr 12, 2022
4afc3d6
Generate correct block editor styles
tellthemachines Apr 12, 2022
72c72ec
Fix tests and add prop to core theme.json
tellthemachines Apr 12, 2022
a26beda
Fix unit tests properly this time
tellthemachines Apr 12, 2022
2afe030
Merge remote-tracking branch 'origin/trunk' into try/root-padding-fix
ramonjd Apr 26, 2022
81b4578
Merge remote-tracking branch 'origin/trunk' into try/root-padding-fix
ramonjd Apr 26, 2022
0218267
Added missing doc comment for parameter "$use_root_vars"
ramonjd Apr 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/block-supports/layout.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ function gutenberg_get_layout_style( $selector, $layout, $has_block_gap_support
$wide_max_width_value = wp_strip_all_tags( explode( ';', $wide_max_width_value )[0] );

if ( $content_size || $wide_size ) {
$style = "$selector > :where(:not(.alignleft):not(.alignright)) {";
$style = "$selector > :where(:not(.alignleft):not(.alignright):not(.alignfull)) {";
$style .= 'max-width: ' . esc_html( $all_max_width_value ) . ';';
$style .= 'margin-left: auto !important;';
$style .= 'margin-right: auto !important;';
Expand Down
262 changes: 208 additions & 54 deletions lib/compat/wordpress-6.0/class-wp-theme-json-gutenberg.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,49 +22,54 @@ class WP_Theme_JSON_Gutenberg extends WP_Theme_JSON_5_9 {
* path to the value in theme.json & block attributes.
*/
const PROPERTIES_METADATA = array(
'background' => array( 'color', 'gradient' ),
'background-color' => array( 'color', 'background' ),
'border-radius' => array( 'border', 'radius' ),
'border-top-left-radius' => array( 'border', 'radius', 'topLeft' ),
'border-top-right-radius' => array( 'border', 'radius', 'topRight' ),
'border-bottom-left-radius' => array( 'border', 'radius', 'bottomLeft' ),
'border-bottom-right-radius' => array( 'border', 'radius', 'bottomRight' ),
'border-color' => array( 'border', 'color' ),
'border-width' => array( 'border', 'width' ),
'border-style' => array( 'border', 'style' ),
'border-top-color' => array( 'border', 'top', 'color' ),
'border-top-width' => array( 'border', 'top', 'width' ),
'border-top-style' => array( 'border', 'top', 'style' ),
'border-right-color' => array( 'border', 'right', 'color' ),
'border-right-width' => array( 'border', 'right', 'width' ),
'border-right-style' => array( 'border', 'right', 'style' ),
'border-bottom-color' => array( 'border', 'bottom', 'color' ),
'border-bottom-width' => array( 'border', 'bottom', 'width' ),
'border-bottom-style' => array( 'border', 'bottom', 'style' ),
'border-left-color' => array( 'border', 'left', 'color' ),
'border-left-width' => array( 'border', 'left', 'width' ),
'border-left-style' => array( 'border', 'left', 'style' ),
'color' => array( 'color', 'text' ),
'font-family' => array( 'typography', 'fontFamily' ),
'font-size' => array( 'typography', 'fontSize' ),
'font-style' => array( 'typography', 'fontStyle' ),
'font-weight' => array( 'typography', 'fontWeight' ),
'letter-spacing' => array( 'typography', 'letterSpacing' ),
'line-height' => array( 'typography', 'lineHeight' ),
'margin' => array( 'spacing', 'margin' ),
'margin-top' => array( 'spacing', 'margin', 'top' ),
'margin-right' => array( 'spacing', 'margin', 'right' ),
'margin-bottom' => array( 'spacing', 'margin', 'bottom' ),
'margin-left' => array( 'spacing', 'margin', 'left' ),
'padding' => array( 'spacing', 'padding' ),
'padding-top' => array( 'spacing', 'padding', 'top' ),
'padding-right' => array( 'spacing', 'padding', 'right' ),
'padding-bottom' => array( 'spacing', 'padding', 'bottom' ),
'padding-left' => array( 'spacing', 'padding', 'left' ),
'--wp--style--block-gap' => array( 'spacing', 'blockGap' ),
'text-decoration' => array( 'typography', 'textDecoration' ),
'text-transform' => array( 'typography', 'textTransform' ),
'filter' => array( 'filter', 'duotone' ),
'background' => array( 'color', 'gradient' ),
'background-color' => array( 'color', 'background' ),
'border-radius' => array( 'border', 'radius' ),
'border-top-left-radius' => array( 'border', 'radius', 'topLeft' ),
'border-top-right-radius' => array( 'border', 'radius', 'topRight' ),
'border-bottom-left-radius' => array( 'border', 'radius', 'bottomLeft' ),
'border-bottom-right-radius' => array( 'border', 'radius', 'bottomRight' ),
'border-color' => array( 'border', 'color' ),
'border-width' => array( 'border', 'width' ),
'border-style' => array( 'border', 'style' ),
'border-top-color' => array( 'border', 'top', 'color' ),
'border-top-width' => array( 'border', 'top', 'width' ),
'border-top-style' => array( 'border', 'top', 'style' ),
'border-right-color' => array( 'border', 'right', 'color' ),
'border-right-width' => array( 'border', 'right', 'width' ),
'border-right-style' => array( 'border', 'right', 'style' ),
'border-bottom-color' => array( 'border', 'bottom', 'color' ),
'border-bottom-width' => array( 'border', 'bottom', 'width' ),
'border-bottom-style' => array( 'border', 'bottom', 'style' ),
'border-left-color' => array( 'border', 'left', 'color' ),
'border-left-width' => array( 'border', 'left', 'width' ),
'border-left-style' => array( 'border', 'left', 'style' ),
'color' => array( 'color', 'text' ),
'font-family' => array( 'typography', 'fontFamily' ),
'font-size' => array( 'typography', 'fontSize' ),
'font-style' => array( 'typography', 'fontStyle' ),
'font-weight' => array( 'typography', 'fontWeight' ),
'letter-spacing' => array( 'typography', 'letterSpacing' ),
'line-height' => array( 'typography', 'lineHeight' ),
'margin' => array( 'spacing', 'margin' ),
'margin-top' => array( 'spacing', 'margin', 'top' ),
'margin-right' => array( 'spacing', 'margin', 'right' ),
'margin-bottom' => array( 'spacing', 'margin', 'bottom' ),
'margin-left' => array( 'spacing', 'margin', 'left' ),
'padding' => array( 'spacing', 'padding' ),
'padding-top' => array( 'spacing', 'padding', 'top' ),
'padding-right' => array( 'spacing', 'padding', 'right' ),
'padding-bottom' => array( 'spacing', 'padding', 'bottom' ),
'padding-left' => array( 'spacing', 'padding', 'left' ),
'--wp--style--root--padding' => array( 'spacing', 'padding' ),
'--wp--style--root--padding-top' => array( 'spacing', 'padding', 'top' ),
'--wp--style--root--padding-right' => array( 'spacing', 'padding', 'right' ),
'--wp--style--root--padding-bottom' => array( 'spacing', 'padding', 'bottom' ),
'--wp--style--root--padding-left' => array( 'spacing', 'padding', 'left' ),
'--wp--style--block-gap' => array( 'spacing', 'blockGap' ),
'text-decoration' => array( 'typography', 'textDecoration' ),
'text-transform' => array( 'typography', 'textTransform' ),
'filter' => array( 'filter', 'duotone' ),
);

/**
Expand Down Expand Up @@ -202,14 +207,15 @@ class WP_Theme_JSON_Gutenberg extends WP_Theme_JSON_5_9 {
* @var array
*/
const VALID_SETTINGS = array(
'appearanceTools' => null,
'border' => array(
'appearanceTools' => null,
'useRootVariables' => null,
'border' => array(
'color' => null,
'radius' => null,
'style' => null,
'width' => null,
),
'color' => array(
'color' => array(
'background' => null,
'custom' => null,
'customDuotone' => null,
Expand All @@ -223,18 +229,18 @@ class WP_Theme_JSON_Gutenberg extends WP_Theme_JSON_5_9 {
'palette' => null,
'text' => null,
),
'custom' => null,
'layout' => array(
'custom' => null,
'layout' => array(
'contentSize' => null,
'wideSize' => null,
),
'spacing' => array(
'spacing' => array(
'blockGap' => null,
'margin' => null,
'padding' => null,
'units' => null,
),
'typography' => array(
'typography' => array(
'customFontSize' => null,
'dropCap' => null,
'fontFamilies' => null,
Expand Down Expand Up @@ -325,10 +331,11 @@ protected function get_block_classes( $style_nodes ) {
continue;
}

$node = _wp_array_get( $this->theme_json, $metadata['path'], array() );
$selector = $metadata['selector'];
$settings = _wp_array_get( $this->theme_json, array( 'settings' ) );
$declarations = static::compute_style_properties( $node, $settings );
$use_root_vars = _wp_array_get( $this->theme_json, array( 'settings', 'useRootVariables' ), array() );
$node = _wp_array_get( $this->theme_json, $metadata['path'], array() );
$selector = $metadata['selector'];
$settings = _wp_array_get( $this->theme_json, array( 'settings' ) );
$declarations = static::compute_style_properties( $node, $settings, null, $selector, $use_root_vars );

// 1. Separate the ones who use the general selector
// and the ones who use the duotone selector.
Expand Down Expand Up @@ -362,6 +369,16 @@ protected function get_block_classes( $style_nodes ) {
}

if ( static::ROOT_BLOCK_SELECTOR === $selector ) {
if ( $use_root_vars ) {
$block_rules .= '.wp-site-blocks { padding-top: var(--wp--style--root--padding-top); padding-bottom: var(--wp--style--root--padding-bottom); }';
$block_rules .= '.wp-site-blocks > * { padding-right: var(--wp--style--root--padding-right); padding-left: var(--wp--style--root--padding-left); }';
$block_rules .= '.wp-site-blocks > * > .alignfull { margin-right: calc(var(--wp--style--root--padding-right) * -1); margin-left: calc(var(--wp--style--root--padding-left) * -1); }';

// Alignfull blocks in the block editor that are direct children of post content should also get negative margins.
if ( is_callable( 'get_current_screen' ) && get_current_screen()->is_block_editor() ) {
$block_rules .= '.is-root-container > .alignfull { margin-right: calc(var(--wp--style--root--padding-right) * -1); margin-left: calc(var(--wp--style--root--padding-left) * -1); }';
}
}
$block_rules .= '.wp-site-blocks > .alignleft { float: left; margin-right: 2em; }';
$block_rules .= '.wp-site-blocks > .alignright { float: right; margin-left: 2em; }';
$block_rules .= '.wp-site-blocks > .aligncenter { justify-content: center; margin-left: auto; margin-right: auto; }';
Expand Down Expand Up @@ -529,6 +546,143 @@ protected static function get_metadata_boolean( $data, $path, $default = false )
return $default;
}

/**
* Given a styles array, it extracts the style properties
* and adds them to the $declarations array following the format:
*
* ```php
* array(
* 'name' => 'property_name',
* 'value' => 'property_value,
* )
* ```
*
* @param array $styles Styles to process.
* @param array $settings Theme settings.
* @param array $properties Properties metadata.
* @param string $selector Selector for styles.
* @param boolean $use_root_vars Whether to use root variables.
* @return array Returns the modified $declarations.
*/
protected static function compute_style_properties( $styles, $settings = array(), $properties = null, $selector = null, $use_root_vars = null ) {
if ( null === $properties ) {
$properties = static::PROPERTIES_METADATA;
}

$declarations = array();
$root_variable_duplicates = array();

if ( empty( $styles ) ) {
return $declarations;
}

foreach ( $properties as $css_property => $value_path ) {
$value = static::get_property_value( $styles, $value_path );

if ( strpos( $css_property, '--wp--style--root--' ) === 0 && static::ROOT_BLOCK_SELECTOR !== $selector ) {
continue;
}

if ( strpos( $css_property, '--wp--style--root--' ) === 0 && $use_root_vars ) {
$root_variable_duplicates[] = substr( $css_property, strlen( '--wp--style--root--' ) );
}

// Root padding requires special logic to split shorthand values.
if ( '--wp--style--root--padding' === $css_property && is_string( $value ) ) {

$shorthand_top = '0';
$shorthand_right = '0';
$shorthand_bottom = '0';
$shorthand_left = '0';

$separate_values = explode( ' ', $value );
tellthemachines marked this conversation as resolved.
Show resolved Hide resolved

switch ( count( $separate_values ) ) {
case 1:
$shorthand_top = $separate_values[0];
$shorthand_right = $separate_values[0];
$shorthand_bottom = $separate_values[0];
$shorthand_left = $separate_values[0];
break;
case 2:
$shorthand_top = $separate_values[0];
$shorthand_right = $separate_values[1];
$shorthand_bottom = $separate_values[0];
$shorthand_left = $separate_values[1];
break;
case 3:
$shorthand_top = $separate_values[0];
$shorthand_right = $separate_values[1];
$shorthand_bottom = $separate_values[2];
$shorthand_left = $separate_values[1];
break;
case 4:
$shorthand_top = $separate_values[0];
$shorthand_right = $separate_values[1];
$shorthand_bottom = $separate_values[2];
$shorthand_left = $separate_values[3];
break;
}

$all_properties = array(
array(
'name' => '--wp--style--root--padding-top',
'value' => $shorthand_top,
),
array(
'name' => '--wp--style--root--padding-right',
'value' => $shorthand_right,
),
array(
'name' => '--wp--style--root--padding-bottom',
'value' => $shorthand_bottom,
),
array(
'name' => '--wp--style--root--padding-left',
'value' => $shorthand_left,
),
);

$declarations = array_merge( $declarations, $all_properties );

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if it's been brought up yet, but this is a lot of bespoke code for root padding values future iterations. I'm not sure where we'd put it in future iterations.

I'm not saying it's a blocker, just curious to learn whether we need a separate processing method for root block selectors to keep these methods uncluttered.

I'd speculate that root style CSS vars could be useful elsewhere, e.g., --wp--style--root--block-gap

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I agree a separate method for root selectors might make this tidier.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

continue;
}

// Look up protected properties, keyed by value path.
// Skip protected properties that are explicitly set to `null`.
if ( is_array( $value_path ) ) {
$path_string = implode( '.', $value_path );
if (
array_key_exists( $path_string, static::PROTECTED_PROPERTIES ) &&
_wp_array_get( $settings, static::PROTECTED_PROPERTIES[ $path_string ], null ) === null
) {
continue;
}
}

// Skip if empty and not "0" or value represents array of longhand values.
$has_missing_value = empty( $value ) && ! is_numeric( $value );
if ( $has_missing_value || is_array( $value ) ) {
continue;
}

$declarations[] = array(
'name' => $css_property,
'value' => $value,
);
}

// If a variable value is added to the root, the corresponding property should be removed.
foreach ( $root_variable_duplicates as $duplicate ) {
Copy link
Member

@ramonjd ramonjd Apr 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been playing with an alternative for block gap, in which I've created a second array const ROOT_PROPERTIES_METADATA that contains all the possible root properties.

In this case it would look like this:

const ROOT_PROPERTIES_METADATA = array(
    // all the rest here...
    // except "padding", "padding-top" etc, which are replaced by the following CSS vars
    '--wp--style--root--padding' => array( 'spacing', 'padding' ),
    '--wp--style--root--padding-top' => array( 'spacing', 'padding', 'top' ),
    '--wp--style--root--padding-right' => array( 'spacing', 'padding', 'right' ),
    '--wp--style--root--padding-bottom' => array( 'spacing', 'padding', 'bottom' ),
    '--wp--style--root--padding-left' => array( 'spacing', 'padding', 'left' ),
);

Then the only change to compute_style_properties() method would be:

f ( null === $properties ) {
    $properties =  static::ROOT_BLOCK_SELECTOR === $selector ? static::ROOT_PROPERTIES_METADATA : static::PROPERTIES_METADATA;
}

Just noting for reference! It might be completely bonkers.

Seems to work for padding at least 😆 but haven't battle tested it yet. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooooh that's a great idea! Much neater logic.

$discard = array_search( $duplicate, array_column( $declarations, 'name' ), true );
if ( $discard ) {
array_splice( $declarations, $discard, 1 );
}
}

return $declarations;
}

/**
* Returns a valid theme.json as provided by a theme.
*
Expand Down
1 change: 1 addition & 0 deletions lib/compat/wordpress-6.0/class-wp-theme-json-resolver-6-0.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
* @access private
*/
class WP_Theme_JSON_Resolver_6_0 extends WP_Theme_JSON_Resolver_5_9 {

/**
* Given a theme.json structure modifies it in place
* to update certain values by its translated strings
Expand Down
11 changes: 10 additions & 1 deletion lib/compat/wordpress-6.0/theme.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
"version": 2,
"settings": {
"appearanceTools": false,
"useRootVariables": false,
"border": {
"color": false,
"radius": false,
Expand Down Expand Up @@ -240,6 +241,14 @@
}
},
"styles": {
"spacing": { "blockGap": "24px" }
"spacing": {
"blockGap": "24px",
"padding": {
"top": "var(--wp--style--block-gap)",
"right": "var(--wp--style--block-gap)",
"bottom": "var(--wp--style--block-gap)",
"left": "var(--wp--style--block-gap)"
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "padding" object is the only real addition to this file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this like a demo padding that applies to all themes? won't this be a breaking change for existing themes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I guess it could be if the theme doesn't define any root padding. I was mainly thinking of a fix for #35884, but perhaps that should be post editor specific.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is problem for themes that aren't opting in to site padding. I think we'll need to add it programatically.

}
}
}
3 changes: 3 additions & 0 deletions packages/block-editor/src/hooks/style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,9 @@ export function getInlineStyles( styles = {} ) {
const ignoredStyles = [ 'spacing.blockGap' ];
const output = {};
Object.keys( STYLE_PROPERTY ).forEach( ( propKey ) => {
if ( STYLE_PROPERTY[ propKey ].rootOnly ) {
return;
}
const path = STYLE_PROPERTY[ propKey ].value;
const subPaths = STYLE_PROPERTY[ propKey ].properties;
// Ignore styles on elements because they are handled on the server.
Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/layouts/flow.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ export default {
? `
${ appendSelectors(
selector,
'> :where(:not(.alignleft):not(.alignright))'
'> :where(:not(.alignleft):not(.alignright):not(.alignfull))'
) } {
max-width: ${ contentSize ?? wideSize };
margin-left: auto !important;
Expand Down
11 changes: 11 additions & 0 deletions packages/blocks/src/api/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,17 @@ export const __EXPERIMENTAL_STYLE_PROPERTY = {
value: [ 'spacing', 'blockGap' ],
support: [ 'spacing', 'blockGap' ],
},
'--wp--style--root--padding': {
value: [ 'spacing', 'padding' ],
support: [ 'spacing', 'padding' ],
properties: {
'--wp--style--root--padding-top': 'top',
'--wp--style--root--padding-right': 'right',
'--wp--style--root--padding-bottom': 'bottom',
'--wp--style--root--padding-left': 'left',
},
rootOnly: true,
},
};

export const __EXPERIMENTAL_ELEMENTS = {
Expand Down
Loading