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.

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

Site Title: Add support settings for colors, fonts, and line height #23007

Merged
merged 49 commits into from
Jul 13, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
805d34b
Add experimental support settings for colors, fonts, and line height.
apeatling Jun 8, 2020
05d22e8
Add initial support for rendering of classes and styles for blocks re…
apeatling Jun 11, 2020
d652a9f
Remove custom item justification attribute.
apeatling Jun 11, 2020
357942c
Remove nav block references
apeatling Jun 11, 2020
a2db6d0
add line height support
Addison-Stavlo Jun 11, 2020
3a3095b
consolodate colors arrays
Addison-Stavlo Jun 12, 2020
3eb8e4d
check support, return early if none
Addison-Stavlo Jun 12, 2020
871a272
add support checks for colors
Addison-Stavlo Jun 12, 2020
fa6567b
support checks for typography
Addison-Stavlo Jun 12, 2020
76bc4f8
update comments
Addison-Stavlo Jun 12, 2020
efc7c7b
update function docs
Addison-Stavlo Jun 12, 2020
7e659a9
fix errors/warnings
Addison-Stavlo Jun 12, 2020
c215ae9
formatting
Addison-Stavlo Jun 12, 2020
0d40bad
started a test
Addison-Stavlo Jun 13, 2020
229dcd6
moved to own file
Addison-Stavlo Jun 15, 2020
e45898d
first test passes
Addison-Stavlo Jun 15, 2020
977384b
styles test
Addison-Stavlo Jun 15, 2020
e1cfeaa
unsupported colors test
Addison-Stavlo Jun 15, 2020
680a694
php format
Addison-Stavlo Jun 15, 2020
65c9763
add existing classes/styles
Addison-Stavlo Jun 15, 2020
9ec8584
add class check for style test
Addison-Stavlo Jun 15, 2020
5ca9b47
link color tests
Addison-Stavlo Jun 15, 2020
a9b1e3e
added gradient tests
Addison-Stavlo Jun 15, 2020
732e255
refactor an assertion function
Addison-Stavlo Jun 15, 2020
160d86f
added fontSize tests
Addison-Stavlo Jun 15, 2020
c958549
lineHeight tests
Addison-Stavlo Jun 15, 2020
e089533
test all supports together
Addison-Stavlo Jun 15, 2020
d493142
update comments
Addison-Stavlo Jun 15, 2020
2395a3e
file rename
Addison-Stavlo Jun 15, 2020
fec7628
fix linter errors
Addison-Stavlo Jun 15, 2020
d62406e
more lint error/warning goo
Addison-Stavlo Jun 15, 2020
32bd8bb
lint ignore our secret sauce
Addison-Stavlo Jun 16, 2020
f909ab2
Remove error suppression and return block content on invalid DOM load…
apeatling Jun 16, 2020
1bf2fc0
Update documentation to add site-title block.
apeatling Jun 16, 2020
bf1e768
helper fn and lint fix
Addison-Stavlo Jun 17, 2020
e81946e
use isset for checking the nested style objects
Addison-Stavlo Jun 17, 2020
8923bd8
Update lib/blocks.php
Addison-Stavlo Jun 17, 2020
a581770
Update lib/blocks.php
Addison-Stavlo Jun 17, 2020
7cb2693
check for ending semi-colon on existing styles
Addison-Stavlo Jun 19, 2020
543fb7e
ensure space for string concat and unique-ify-ing
Addison-Stavlo Jun 22, 2020
806a23f
Remove template part block check, this is no longer needed because we…
apeatling Jun 22, 2020
998f7f6
WordPress coding standards tweaks
apeatling Jun 22, 2020
0559112
refactor per Riad's interface suggestion
Addison-Stavlo Jun 24, 2020
35a5856
merge branch 'master' into add/site-title-fonts-colors
Addison-Stavlo Jun 25, 2020
aa4bbdc
cleanup grossly long 1-liners
Addison-Stavlo Jun 25, 2020
1987286
php format
Addison-Stavlo Jun 25, 2020
ba6a395
update tests for better inline style spacing
Addison-Stavlo Jun 25, 2020
7c17c76
require render_callback
Addison-Stavlo Jun 25, 2020
9e148d1
rebase
Addison-Stavlo Jul 1, 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
15 changes: 15 additions & 0 deletions docs/designers-developers/developers/themes/theme-json.md
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ Each block will declare which style properties it exposes. This has been coined
| Group | Yes | Yes | Yes | Yes |
| Columns | Yes | Yes | Yes | Yes |
| Media & text | Yes | Yes | Yes | Yes |
| Site Title | Yes | Yes | - | Yes |

[1] The heading block represents 6 distinct HTML elements: H1-H6. It comes with selectors to target each individual element (ex: core/heading/h1 for H1, etc).

Expand All @@ -180,6 +181,7 @@ Each block will declare which style properties it exposes. This has been coined
| Group | - | - |
| Columns | - | - |
| Media & text | - | - |
| Site Title | Yes | Yes |

[1] The heading block represents 6 distinct HTML elements: H1-H6. It comes with selectors to target each individual element (ex: core/heading/h1 for H1, etc).

Expand Down Expand Up @@ -379,6 +381,19 @@ The list of features that are currently supported are:
"text": <value>
}
}
},
"core/site-title": {
"styles": {
"color": {
"background": <value>,
"gradient": <value>,
"text": <value>
},
"typography": {
"fontSize": <value>,
"lineHeight": <value>
}
}
}
}
```
186 changes: 186 additions & 0 deletions lib/blocks.php
Original file line number Diff line number Diff line change
Expand Up @@ -216,3 +216,189 @@ function gutenberg_register_legacy_social_link_blocks() {
}
}
add_action( 'init', 'gutenberg_register_legacy_social_link_blocks' );

/**
* Renders the classNames and styles for blocks
*
* @param string $block_content Output of the current block.
* @param array $block Block Object.
* @return string New block output.
*/
function gutenberg_experimental_apply_classnames_and_styles( $block_content, $block ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens with existing blocks that already apply these flags on the frontend on their "save" function? how can we exclude them from this behavior?

Copy link
Contributor

Choose a reason for hiding this comment

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

what happens with existing blocks that already apply these flags on the frontend on their "save" function?

They go through this function unnecessarily. They do 'play nice', although this function handling them as well is a bit redundant.

how can we exclude them from this behavior?

Im not sure. There doesn't seem to be anything on the block object passed into the function that signifies anything special about this case. Maybe we can add an extra property on the block object or an attr to signal that it should be ignored by this function? Im open to ideas...

Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't testing for the presence of "render_callback" enough maybe?

Copy link
Contributor

Choose a reason for hiding this comment

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

That seems to do the trick! Thank you 😄 - 7c17c76

if ( ! isset( $block['attrs'] ) ) {
return $block_content;
}

$block_type = WP_Block_Type_Registry::get_instance()->get_registered( $block['blockName'] );
// If no render_callback, assume styles have been previously handled.
if ( ! $block_type || ! $block_type->render_callback ) {
return $block_content;
}
// Check what style features the block supports.
$supports = gutenberg_experimental_global_styles_get_supported_styles( $block_type->supports );

$attributes = array();
$attributes = gutenberg_experimental_build_css_colors( $attributes, $block['attrs'], $supports );
$attributes = gutenberg_experimental_build_css_typography( $attributes, $block['attrs'], $supports );

if ( ! count( $attributes ) ) {
return $block_content;
}

$dom = new DOMDocument( '1.0', 'utf-8' );

// Suppress warnings from this method from polluting the front-end.
// @codingStandardsIgnoreStart
if ( ! @$dom->loadHTML( $block_content, LIBXML_HTML_NOIMPLIED | LIBXML_HTML_NODEFDTD | LIBXML_COMPACT ) ) {
Comment on lines +248 to +252
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for reference: this introduced #24445, with a likely fix in #24447. I don't find it surprising that we discover quirks in these PHP interfaces as we start doing more server-side manipulation of content. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching it. 👍

// @codingStandardsIgnoreEnd
return $block_content;
}

$xpath = new DOMXPath( $dom );
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if regex is more performant. This is definitely stronger and safer but in the long run, performance is important for the frontend. I'm happy starting with this and monitor impact

Copy link
Contributor

Choose a reason for hiding this comment

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

I had a similar thought about performance, but attempting to parse HTML with a regex gets messy really quickly.

FWIW, I found myself in a similar situation a while ago, when adding a new sniff to WPCS. I ended up using XMLReader which out of a number of options seemed to have the smallest resource footprint. However, I only needed parsing at the time -- I didn't have to write HTML.

For writing, there's however XMLWriter -- maybe it could be used in combination with XMLReader.

Here's a StackOverflow post listing PHP's built-in (and external) HTML parsing options.

Here's some more background on RegEx-based parsing (and why it's messy) and alternatives.

$block_root = $xpath->query( '/*' )[0];

if ( empty( $block_root ) ) {
return $block_content;
}

// Some inline styles may be added without ending ';', add this if necessary.
$current_styles = trim( $block_root->getAttribute( 'style' ), ' ' );
if ( strlen( $current_styles ) > 0 && substr( $current_styles, -1 ) !== ';' ) {
$current_styles = $current_styles . ';';
};

// Merge and dedupe new and existing classes and styles.
$classes_to_add = esc_attr( implode( ' ', array_key_exists( 'css_classes', $attributes ) ? $attributes['css_classes'] : array() ) );
$styles_to_add = esc_attr( implode( ' ', array_key_exists( 'inline_styles', $attributes ) ? $attributes['inline_styles'] : array() ) );
$new_classes = implode( ' ', array_unique( explode( ' ', ltrim( $block_root->getAttribute( 'class' ) . ' ' ) . $classes_to_add ) ) );
$new_styles = implode( ' ', array_unique( explode( ' ', $current_styles . ' ' . $styles_to_add ) ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

If I am reading this correctly, the separator for implode and explode should be ';' instead of ' ', otherwise array_unique doesn't have much to trim.

Same for two lines above at the assignment of $styles_to_add.

Using a print_r and a die on my env:

INPUT:
background-color:#b44343;min-height:244px;

OUTPUT:
Array
(
    [0] => background-color:#b44343;min-height:244px;
    [1] => 
)

Copy link
Contributor

@Addison-Stavlo Addison-Stavlo Aug 10, 2020

Choose a reason for hiding this comment

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

Do the styles in $current_styles never have spaces between them like in your example input? or could they be either case? Input: background-color:#b44343;min-height:244px; vs Input: background-color:#b44343; min-height:244px;

It does look like we need to update these so that they are more consistent with each other and can properly remove the duplicates. We can implode $styles_to_add with no spacing (since ';' is already added to the values in the array), and explode/implode $new_styles on ';' if there are never spaces. If there can be I guess we need an extra step to trim whitespace for the array_unique to compare properly? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Do the styles in $current_styles never have spaces between them like in your example input? or could they be either case?

I didn't check to see if this was guaranteeable, but those are the styles I got after using the actual user-facing tools. I'd look at the framework to make sure. If it seems like spaces could make their way in, you can always use preg_split:

php > var_dump( preg_split( '/; ?/', 'color:red;color:green; color:blue' ) );
array(3) {
  [0]=>
  string(9) "color:red"
  [1]=>
  string(11) "color:green"
  [2]=>
  string(10) "color:blue"
}

Of course, you can take it as far as you like:

php > var_dump( preg_split( '/ *; */', 'color:red   ; color:green; color:blue' ) );
array(3) {
  [0]=>
  string(9) "color:red"
  [1]=>
  string(11) "color:green"
  [2]=>
  string(10) "color:blue"
}

Copy link
Contributor

Choose a reason for hiding this comment

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

We can implode $styles_to_add with no spacing (since ';' is already added to the values in the array)

I'm not sure what you mean here. When wouldn't explode/preg_split strip semi-colons from the captured styles?

Copy link
Contributor

@Addison-Stavlo Addison-Stavlo Aug 10, 2020

Choose a reason for hiding this comment

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

Ah yeah preg_split could be the way to go then.

I'm not sure what you mean here. When wouldn't explode/preg_split strip semi-colons from the captured styles?

I just mean that currently the values added to $attributes['inline-styles'] all currently have the semicolons at the end of the string already. So that array looks something like [ '--wp--style--color--link: #fff;', 'background: #000;' ] etc. So when implode happens on the line with $styles_to_add that turns into '--wp--style--color--link: #fff; background: #000;' etc, even though we are just imploding with ' ' as the separator. It looks like with those internal spaces in the values that the explode by ' ' is even more problematic than noted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for pointing this out by the way. I hope to address this relatively shortly but have been preoccupied so far today.

Copy link
Contributor

Choose a reason for hiding this comment

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

I proposed a fix in #24486


// Apply new styles and classes.
if ( ! empty( $new_classes ) ) {
$block_root->setAttribute( 'class', $new_classes );
}

if ( ! empty( $new_styles ) ) {
$block_root->setAttribute( 'style', $new_styles );
}

return $dom->saveHtml();
}
add_filter( 'render_block', 'gutenberg_experimental_apply_classnames_and_styles', 10, 2 );

/**
* Build an array with CSS classes and inline styles defining the colors
* which will be applied to the block markup in the front-end.
*
* @param array $attributes comprehensive list of attributes to be applied.
* @param array $block_attributes block attributes.
* @param array $supports style features the block attributes.
* @return array Colors CSS classes and inline styles.
*/
function gutenberg_experimental_build_css_colors( $attributes, $block_attributes, $supports ) {
// Text Colors.
// Check support for text colors.
if ( in_array( 'color', $supports, true ) ) {
$has_named_text_color = array_key_exists( 'textColor', $block_attributes );
$has_custom_text_color = isset( $block_attributes['style']['color']['text'] );

// Apply required generic class.
if ( $has_custom_text_color || $has_named_text_color ) {
$attributes['css_classes'][] = 'has-text-color';
}
// Apply color class or inline style.
if ( $has_named_text_color ) {
$attributes['css_classes'][] = sprintf( 'has-%s-color', $block_attributes['textColor'] );
} elseif ( $has_custom_text_color ) {
$attributes['inline_styles'][] = sprintf( 'color: %s;', $block_attributes['style']['color']['text'] );
}
}

// Link Colors.
if ( in_array( 'link-color', $supports, true ) ) {
$has_link_color = isset( $block_attributes['style']['color']['link'] );
// Apply required class and style.
if ( $has_link_color ) {
$attributes['css_classes'][] = 'has-link-color';
// If link is a named color.
if ( strpos( $block_attributes['style']['color']['link'], 'var:preset|color|' ) !== false ) {
// Get the name from the string and add proper styles.
$index_to_splice = strrpos( $block_attributes['style']['color']['link'], '|' ) + 1;
$link_color_name = substr( $block_attributes['style']['color']['link'], $index_to_splice );
$attributes['inline_styles'][] = sprintf( '--wp--style--color--link:var(--wp--preset--color--%s);', $link_color_name );
} else {
$attributes['inline_styles'][] = sprintf( '--wp--style--color--link: %s;', $block_attributes['style']['color']['link'] );
}
}
}

// Background Colors.
if ( in_array( 'background-color', $supports, true ) ) {
$has_named_background_color = array_key_exists( 'backgroundColor', $block_attributes );
$has_custom_background_color = isset( $block_attributes['style']['color']['background'] );

// Apply required background class.
if ( $has_custom_background_color || $has_named_background_color ) {
$attributes['css_classes'][] = 'has-background';
}
// Apply background color classes or styles.
if ( $has_named_background_color ) {
$attributes['css_classes'][] = sprintf( 'has-%s-background-color', $block_attributes['backgroundColor'] );
} elseif ( $has_custom_background_color ) {
$attributes['inline_styles'][] = sprintf( 'background-color: %s;', $block_attributes['style']['color']['background'] );
}
}

// Gradients.
if ( in_array( 'background', $supports, true ) ) {
$has_named_gradient = array_key_exists( 'gradient', $block_attributes );
$has_custom_gradient = isset( $block_attributes['style']['color']['gradient'] );

if ( $has_named_gradient || $has_custom_gradient ) {
$attributes['css_classes'][] = 'has-background';
}
// Apply required background class.
if ( $has_named_gradient ) {
$attributes['css_classes'][] = sprintf( 'has-%s-gradient-background', $block_attributes['gradient'] );
} elseif ( $has_custom_gradient ) {
$attributes['inline_styles'][] = sprintf( 'background: %s;', $block_attributes['style']['color']['gradient'] );
}
}
Addison-Stavlo marked this conversation as resolved.
Show resolved Hide resolved

return $attributes;
}

/**
* Build an array with CSS classes and inline styles defining the font sizes
* which will be applied to the block markup in the front-end.
*
* @param array $attributes comprehensive list of attributes to be applied.
* @param array $block_attributes block attributes.
* @param array $supports style features the block attributes.
* @return array Font size CSS classes and inline styles.
*/
function gutenberg_experimental_build_css_typography( $attributes, $block_attributes, $supports ) {
// Font Size.
if ( in_array( 'font-size', $supports, true ) ) {
$has_named_font_size = array_key_exists( 'fontSize', $block_attributes );
$has_custom_font_size = isset( $block_attributes['style']['typography']['fontSize'] );

// Apply required class or style.
if ( $has_named_font_size ) {
$attributes['css_classes'][] = sprintf( 'has-%s-font-size', $block_attributes['fontSize'] );
} elseif ( $has_custom_font_size ) {
$attributes['inline_styles'][] = sprintf( 'font-size: %spx;', $block_attributes['style']['typography']['fontSize'] );
}
}

// Line Height.
if ( in_array( 'line-height', $supports, true ) ) {
$has_line_height = isset( $block_attributes['style']['typography']['lineHeight'] );
// Add the style (no classes for line-height).
if ( $has_line_height ) {
$attributes['inline_styles'][] = sprintf( 'line-height: %s;', $block_attributes['style']['typography']['lineHeight'] );
}
}

return $attributes;
}
Addison-Stavlo marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions lib/global-styles.php
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,7 @@ function gutenberg_experimental_global_styles_get_supported_styles( $supports )
'color' => array( '__experimentalColor' ),
'background-color' => array( '__experimentalColor' ),
'background' => array( '__experimentalColor', 'gradients' ),
'link-color' => array( '__experimentalColor', 'linkColor' ),
'line-height' => array( '__experimentalLineHeight' ),
'font-size' => array( '__experimentalFontSize' ),
);
Expand Down
7 changes: 6 additions & 1 deletion packages/block-library/src/site-title/block.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@
},
"supports": {
"html": false,
"lightBlockWrapper": true
"lightBlockWrapper": true,
"__experimentalColor": {
"gradients": true
},
"__experimentalFontSize": true,
"__experimentalLineHeight": true
apeatling marked this conversation as resolved.
Show resolved Hide resolved
}
}
1 change: 0 additions & 1 deletion phpunit/class-block-context-test.php
Original file line number Diff line number Diff line change
Expand Up @@ -196,5 +196,4 @@ function test_default_context_is_filterable() {

$this->assertEquals( array( 'example' => 'ok' ), $provided_context[0] );
}

}
Loading