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

Add padding control to the Global Styles sidebar #27154

Merged
merged 18 commits into from
Dec 30, 2020
Merged

Add padding control to the Global Styles sidebar #27154

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
8eb649b
Add spacing panel to global styles sidebar
oandregal Nov 19, 2020
7fbbcef
Add check for theme support as well
oandregal Dec 1, 2020
4d677fe
fix: consider name
oandregal Dec 1, 2020
f75e0da
Pass context name to editor feature
oandregal Dec 1, 2020
75341a2
Rename getter & setter for consistency: getStyle/setStyle, getSetting…
oandregal Dec 1, 2020
52bae1d
Process shorthand properties
oandregal Dec 1, 2020
97d8c31
Add tests
oandregal Dec 1, 2020
5a736a5
Declare padding as a shorthand
oandregal Dec 1, 2020
5feafdb
Fix support check
oandregal Dec 1, 2020
2a58dd9
Spacing panel uses new padding property
oandregal Dec 1, 2020
1e23daa
Add support for shorthand properties
oandregal Dec 2, 2020
933d259
Make server & client structures the same
oandregal Dec 2, 2020
d97269f
Adapt inline styles
oandregal Dec 11, 2020
43523bf
Make linter happy
oandregal Dec 14, 2020
5d5cd0d
make linter happy
oandregal Dec 16, 2020
1e08467
Protect against undefined
oandregal Dec 22, 2020
6a4f4f3
Adapt filter to nested properties
oandregal Dec 29, 2020
8f428c3
Make linter happy
oandregal Dec 29, 2020
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
78 changes: 58 additions & 20 deletions lib/class-wp-theme-json.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,21 +272,10 @@ class WP_Theme_JSON {
'value' => array( 'typography', 'lineHeight' ),
'support' => array( 'lineHeight' ),
),
'paddingBottom' => array(
'value' => array( 'spacing', 'padding', 'bottom' ),
'support' => array( 'spacing', 'padding' ),
),
'paddingLeft' => array(
'value' => array( 'spacing', 'padding', 'left' ),
'support' => array( 'spacing', 'padding' ),
),
'paddingRight' => array(
'value' => array( 'spacing', 'padding', 'right' ),
'support' => array( 'spacing', 'padding' ),
),
'paddingTop' => array(
'value' => array( 'spacing', 'padding', 'top' ),
'support' => array( 'spacing', 'padding' ),
'padding' => array(
'value' => array( 'spacing', 'padding' ),
'support' => array( 'spacing', 'padding' ),
'properties' => array( 'top', 'right', 'bottom', 'left' ),
),
'textDecoration' => array(
'value' => array( 'typography', 'textDecoration' ),
Expand Down Expand Up @@ -504,10 +493,25 @@ private static function process_key( $key, &$input, $schema, $should_escape = fa
if ( 'gradient' === $property ) {
$name = 'background';
}
$result = safecss_filter_attr( "$name: $value" );

if ( '' === $result ) {
unset( $input[ $key ][ $property ] );
if ( is_array( $value ) ) {
oandregal marked this conversation as resolved.
Show resolved Hide resolved
$result = array();
foreach ( $value as $subproperty => $subvalue ) {
$result_subproperty = safecss_filter_attr( "$name: $subvalue" );
Copy link
Member

Choose a reason for hiding this comment

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

We are calling safecss_filter_attr with things like:

string(21) "background-color: 8px"
string(22) "background-color: 50px"
string(21) "background-color: 8px"
string(21) "background-color: 9px"

Possible to test by setting a user padding and adding var_dump( "$name: $subvalue" );before safecss_filter_attr filter call.

	$name = 'background-color';
	if ( 'gradient' === $property ) {
		$name = 'background';
	}

Name is always background-color or background we need to make sure the name also accounts for the padding properties.

Copy link
Member Author

@oandregal oandregal Dec 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is intentional, safecss_filter_attr does not check fine-grained semantics of the declaration, it only has 3 cases: 1) the value is fine (does not contain slashes, parenthesis, etc), 2) the value can contain gradients if the property is allowed to have gradients according to kses, and 3) the value can contain URL data if the property is considered safe for that. As per properties, as long they're is in the allowed list, things are fine.

Via theme.json we output more properties than the allowed by the kses function, for example, link color (a CSS Custom Property). I thought the current approach was simpler and equally robust than transforming every property to its CSS form except for link color, which we'd need to transform to another thing.

How do you feel about this?

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR doesn't change this behavior so went ahead and merge it. We can continue to discuss whether this is how we want it to behave.

Copy link
Member

@jorgefilipecosta jorgefilipecosta Jan 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I thought the current approach was simpler and equally robust than transforming every property to its CSS form except for link color, which we'd need to transform to another thing.

How do you feel about this?

Hi @nosolosw, I think we need to update and use the correct CSS properties because using just two hardcoded properties is not something expected. E.g: A plugin may change to allow using filter safecss_filter_attr_allow_css a specific CSS only on a specific property, and with this approach, we don't respect what the plugin wants to do.

if ( '' !== $result_subproperty ) {
$result[ $subproperty ] = $result_subproperty;
}
}

if ( empty( $result ) ) {
unset( $input[ $key ][ $property ] );
}
} else {
$result = safecss_filter_attr( "$name: $value" );

if ( '' === $result ) {
unset( $input[ $key ][ $property ] );
}
}
}
}
Expand Down Expand Up @@ -625,6 +629,21 @@ private static function get_property_value( $styles, $path ) {
return $value;
}

/**
* Whether the medatata contains a key named properties.
*
* @param array $metadata Description of the style property.
*
* @return boolean True if properties exists, false otherwise.
*/
private static function has_properties( $metadata ) {
if ( array_key_exists( 'properties', $metadata ) ) {
return true;
}

return false;
}

/**
* Given a context, it extracts the style properties
* and adds them to the $declarations array following the format:
Expand All @@ -647,14 +666,33 @@ private static function compute_style_properties( &$declarations, $context, $con
return;
}

$properties = array();
foreach ( self::PROPERTIES_METADATA as $name => $metadata ) {
if ( ! in_array( $name, $context_supports, true ) ) {
continue;
}

$value = self::get_property_value( $context['styles'], $metadata['value'] );
// Some properties can be shorthand properties, meaning that
// they contain multiple values instead of a single one.
if ( self::has_properties( $metadata ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

We are producing styles like:

.wp-block-group {
	padding-top: 5px;
	padding-right: 5px;
	padding-bottom: 5px;
	padding-left: 5px;
}

Could we be smart and output something like "padding: 5px" in these cases? To avoiding sending unrequited bytes over the wire?
I guess we can use a "padding" property every time all for properties are set and the same behavior can be applied to margins borders etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

That'd be nice, can we get this in in a follow-up?

foreach ( $metadata['properties'] as $property ) {
$properties[] = array(
'name' => $name . ucfirst( $property ),
'value' => array_merge( $metadata['value'], array( $property ) ),
);
}
} else {
$properties[] = array(
'name' => $name,
'value' => $metadata['value'],
);
}
}

foreach ( $properties as $prop ) {
$value = self::get_property_value( $context['styles'], $prop['value'] );
if ( ! empty( $value ) ) {
$kebabcased_name = strtolower( preg_replace( '/(?<!^)[A-Z]/', '-$0', $name ) );
$kebabcased_name = strtolower( preg_replace( '/(?<!^)[A-Z]/', '-$0', $prop['name'] ) );
$declarations[] = array(
'name' => $kebabcased_name,
'value' => $value,
Expand Down
25 changes: 14 additions & 11 deletions packages/block-editor/src/hooks/style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { has, get, startsWith, mapValues } from 'lodash';
import { capitalize, has, get, startsWith } from 'lodash';

/**
* WordPress dependencies
Expand Down Expand Up @@ -54,18 +54,21 @@ function compileStyleValue( uncompiledValue ) {
*/
export function getInlineStyles( styles = {} ) {
const output = {};
const styleProperties = mapValues( STYLE_PROPERTY, ( prop ) => prop.value );
Object.entries( styleProperties ).forEach(
( [ styleKey, ...otherObjectKeys ] ) => {
const [ objectKeys ] = otherObjectKeys;

if ( has( styles, objectKeys ) ) {
output[ styleKey ] = compileStyleValue(
get( styles, objectKeys )
);
Object.keys( STYLE_PROPERTY ).forEach( ( propKey ) => {
const path = STYLE_PROPERTY[ propKey ].value;
const subPaths = STYLE_PROPERTY[ propKey ].properties;
if ( has( styles, path ) ) {
if ( !! subPaths ) {
subPaths.forEach( ( suffix ) => {
output[
propKey + capitalize( suffix )
] = compileStyleValue( get( styles, [ ...path, suffix ] ) );
} );
} else {
output[ propKey ] = compileStyleValue( get( styles, path ) );
}
}
);
} );

return output;
}
Expand Down
17 changes: 3 additions & 14 deletions packages/blocks/src/api/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,21 +53,10 @@ export const __EXPERIMENTAL_STYLE_PROPERTY = {
value: [ 'typography', 'lineHeight' ],
support: [ 'lineHeight' ],
},
paddingBottom: {
value: [ 'spacing', 'padding', 'bottom' ],
support: [ 'spacing', 'padding' ],
},
paddingLeft: {
value: [ 'spacing', 'padding', 'left' ],
support: [ 'spacing', 'padding' ],
},
paddingRight: {
value: [ 'spacing', 'padding', 'right' ],
support: [ 'spacing', 'padding' ],
},
paddingTop: {
value: [ 'spacing', 'padding', 'top' ],
padding: {
value: [ 'spacing', 'padding' ],
support: [ 'spacing', 'padding' ],
properties: [ 'top', 'right', 'bottom', 'left' ],
},
textDecoration: {
value: [ 'typography', 'textDecoration' ],
Expand Down
10 changes: 4 additions & 6 deletions packages/edit-site/src/components/editor/global-styles-provider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ const GlobalStylesContext = createContext( {
/* eslint-disable no-unused-vars */
getSetting: ( context, path ) => {},
setSetting: ( context, path, newValue ) => {},
getStyleProperty: ( context, propertyName, origin ) => {},
setStyleProperty: ( context, propertyName, newValue ) => {},
getStyle: ( context, propertyName, origin ) => {},
setStyle: ( context, propertyName, newValue ) => {},
contexts: {},
/* eslint-enable no-unused-vars */
} );
Expand Down Expand Up @@ -164,7 +164,7 @@ export default function GlobalStylesProvider( { children, baseStyles } ) {
set( contextSettings, path, newValue );
setContent( JSON.stringify( newContent ) );
},
getStyleProperty: ( context, propertyName, origin = 'merged' ) => {
getStyle: ( context, propertyName, origin = 'merged' ) => {
const styles = 'user' === origin ? userStyles : mergedStyles;

const value = get(
Expand All @@ -173,7 +173,7 @@ export default function GlobalStylesProvider( { children, baseStyles } ) {
);
return getValueFromVariable( mergedStyles, context, value );
},
setStyleProperty: ( context, propertyName, newValue ) => {
setStyle: ( context, propertyName, newValue ) => {
const newContent = { ...userStyles };
let contextStyles = newContent?.[ context ]?.styles;
if ( ! contextStyles ) {
Expand Down Expand Up @@ -208,7 +208,6 @@ export default function GlobalStylesProvider( { children, baseStyles } ) {
css: getGlobalStyles(
contexts,
mergedStyles,
STYLE_PROPERTY,
'cssVariables'
),
isGlobalStyles: true,
Expand All @@ -218,7 +217,6 @@ export default function GlobalStylesProvider( { children, baseStyles } ) {
css: getGlobalStyles(
contexts,
mergedStyles,
STYLE_PROPERTY,
'blockStyles'
),
isGlobalStyles: true,
Expand Down
48 changes: 31 additions & 17 deletions packages/edit-site/src/components/editor/global-styles-renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
/**
* External dependencies
*/
import { get, kebabCase, reduce, startsWith } from 'lodash';
import { capitalize, get, kebabCase, reduce, startsWith } from 'lodash';

/**
* WordPress dependencies
*/
import { __EXPERIMENTAL_STYLE_PROPERTY as STYLE_PROPERTY } from '@wordpress/blocks';

/**
* Internal dependencies
Expand Down Expand Up @@ -96,36 +101,46 @@ function flattenTree( input = {}, prefix, token ) {
*
* @param {Object} blockSupports What styles the block supports.
* @param {Object} blockStyles Block styles.
* @param {Object} metadata Block styles metadata information.
*
* @return {Array} An array of style declarations.
*/
function getBlockStylesDeclarations(
blockSupports,
blockStyles = {},
metadata
) {
function getBlockStylesDeclarations( blockSupports, blockStyles = {} ) {
return reduce(
metadata,
( declarations, { value }, key ) => {
const cssProperty = key.startsWith( '--' ) ? key : kebabCase( key );
if (
blockSupports.includes( key ) &&
get( blockStyles, value, false )
) {
STYLE_PROPERTY,
( declarations, { value, properties }, key ) => {
if ( ! blockSupports.includes( key ) ) {
return declarations;
}

if ( !! properties ) {
properties.forEach( ( prop ) => {
const cssProperty = key.startsWith( '--' )
? key
: kebabCase( `${ key }${ capitalize( prop ) }` );
declarations.push(
`${ cssProperty }: ${ compileStyleValue(
get( blockStyles, [ ...value, prop ] )
) }`
);
} );
} else if ( get( blockStyles, value, false ) ) {
const cssProperty = key.startsWith( '--' )
? key
: kebabCase( key );
declarations.push(
`${ cssProperty }: ${ compileStyleValue(
get( blockStyles, value )
) }`
);
}

return declarations;
},
[]
);
}

export default ( blockData, tree, metadata, type = 'all' ) => {
export default ( blockData, tree, type = 'all' ) => {
return reduce(
blockData,
( styles, { selector }, context ) => {
Expand All @@ -150,8 +165,7 @@ export default ( blockData, tree, metadata, type = 'all' ) => {
if ( type === 'all' || type === 'blockStyles' ) {
const blockStyleDeclarations = getBlockStylesDeclarations(
blockData[ context ].supports,
tree?.[ context ]?.styles,
metadata
tree?.[ context ]?.styles
);

if ( blockStyleDeclarations.length > 0 ) {
Expand Down
34 changes: 14 additions & 20 deletions packages/edit-site/src/components/sidebar/color-panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ export function useHasColorPanel( { supports } ) {

export default function ColorPanel( {
context: { supports, name },
getStyleProperty,
setStyleProperty,
getStyle,
setStyle,
getSetting,
setSetting,
} ) {
Expand All @@ -37,29 +37,24 @@ export default function ColorPanel( {
const settings = [];

if ( supports.includes( 'color' ) ) {
const color = getStyleProperty( name, 'color' );
const userColor = getStyleProperty( name, 'color', 'user' );
const color = getStyle( name, 'color' );
const userColor = getStyle( name, 'color', 'user' );
settings.push( {
colorValue: color,
onColorChange: ( value ) =>
setStyleProperty( name, 'color', value ),
onColorChange: ( value ) => setStyle( name, 'color', value ),
label: __( 'Text color' ),
clearable: color === userColor,
} );
}

let backgroundSettings = {};
if ( supports.includes( 'backgroundColor' ) ) {
const backgroundColor = getStyleProperty( name, 'backgroundColor' );
const userBackgroundColor = getStyleProperty(
name,
'backgroundColor',
'user'
);
const backgroundColor = getStyle( name, 'backgroundColor' );
const userBackgroundColor = getStyle( name, 'backgroundColor', 'user' );
backgroundSettings = {
colorValue: backgroundColor,
onColorChange: ( value ) =>
setStyleProperty( name, 'backgroundColor', value ),
setStyle( name, 'backgroundColor', value ),
};
if ( backgroundColor ) {
backgroundSettings.clearable =
Expand All @@ -69,12 +64,12 @@ export default function ColorPanel( {

let gradientSettings = {};
if ( supports.includes( 'background' ) ) {
const gradient = getStyleProperty( name, 'background' );
const userGradient = getStyleProperty( name, 'background', 'user' );
const gradient = getStyle( name, 'background' );
const userGradient = getStyle( name, 'background', 'user' );
gradientSettings = {
gradientValue: gradient,
onGradientChange: ( value ) =>
setStyleProperty( name, 'background', value ),
setStyle( name, 'background', value ),
};
if ( gradient ) {
gradientSettings.clearable = gradient === userGradient;
Expand All @@ -93,12 +88,11 @@ export default function ColorPanel( {
}

if ( supports.includes( LINK_COLOR ) ) {
const color = getStyleProperty( name, LINK_COLOR );
const userColor = getStyleProperty( name, LINK_COLOR, 'user' );
const color = getStyle( name, LINK_COLOR );
const userColor = getStyle( name, LINK_COLOR, 'user' );
settings.push( {
colorValue: color,
onColorChange: ( value ) =>
setStyleProperty( name, LINK_COLOR, value ),
onColorChange: ( value ) => setStyle( name, LINK_COLOR, value ),
label: __( 'Link color' ),
clearable: color === userColor,
} );
Expand Down
Loading