Site Title: Add support settings for colors, fonts, and line height #23007
Conversation
|
Size Change: +22 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
|
This PR will need server side logic, possibly relevant: #21420 |
|
I've added some initial work to support rendering classNames and inline styles for server side rendered blocks. This works for the Site Title block included in this PR as a test block. Would love some feedback on this, is this heading in the right direction? A few thoughts:
|
|
This is looking great so far! I'm hoping this method is favorable as its nice to not have to deal with the regex.
maybe lib/global-styles.php makes sense (at least if this interacts with that the way I am thinking it may)? I'm sure some of the other's that have been pinged may be able to supply more clarity on that.
I was wondering about this too. Maybe some of the unsupported ones are set up to apply some of these classes or styles to specific elements interior to the block wrapper... in which case this would then apply those same classes or styles to the block wrapper redundantly. |
So far, the things blocks need to play nicely with the block style system (both block-level & global-level) are:
This PR checks both boxes so nothing else is required for it to work. This sort of work is tracked at #22700 and documented in the handbook at |
|
Great work on this PR @Addison-Stavlo! |
|
I've noticed that editing and saving the site title value does not seem to work, however it's also not working in master so seems unrelated to the changes in this PR. |
|
@Addison-Stavlo I think we should also support link color? Although the site title block does not contain a link, I expect it will in the future because users will want the title to link back to their homepage. |
I think supporting link color at this point would be meaningless since we cannot apply a link to the Site Title. Since we cannot use the rich text for the site title to apply a link that way, we would need to add a savable attribute to the block and some case specific tools to its edit.js and index.php to set/apply/render the link? It probably doesn't make sense to add the supports flag for link color unless that other stuff to support the link here is also put in place. That could be a good followup after this PR to improve the block if that link functionality for the site title is desired.
Thats probably some functionality we will also want to add. Id say that would go well as a follow-up, but maybe we could squeeze that in here. |
Yes good point on RichText, I'd completely forgotten that we can't support a link in the same way. Agreed we should hold off until we know a link will be supported.
I think this is a regression, it was working previously if I'm not mistaken. I think we should deal with that in a follow up. |
… are testing for experimental feature support on blocks.
lib/blocks.php
Outdated
| $colors['css_classes'], | ||
| $typography['css_classes'], | ||
| isset( $block['attrs']['className'] ) ? array( $block['attrs']['className'] ) : array(), | ||
| isset( $block['attrs']['align'] ) ? array( 'has-text-align-' . $block['attrs']['align'] ) : array() |
There was a problem hiding this comment.
@youknowriad - I'm wondering if it makes sense for us to add the text alignment and className here?
If it does, should we skip the early return for no support flags and apply align and className for all blocks that have those attributes saved? Or should we only apply these attrs if the block is using one of the experimental support flags? (or not at all?)
Other than that, this PR should be ready for another look over once you get a chance. 😄
There was a problem hiding this comment.
My feeling is that this function knows too much about what each support flag (align, classnames, experimental ones)... does.
Ideally it's written in a generic way where we'd have a clear "interface" if want to add a new support flag. Doest that make sense?
There was a problem hiding this comment.
i seem something like
$attributes = array();
$attributes = gutenberg_experimental_apply_colors_flag( $attributes, $block['attrs'], $supports );
$attributes = gutenberg_experimental_apply_lineheight_flag( $attributes, $block['attrs'], $supports );
$attributes = gutenberg_experimental_apply-align_flag( $attributes, $block['attrs'], $supports );
$attributes = gutenberg_experimental_apply_custom_classname_flag( $attributes, $block['attrs'], $supports );
if ( ! count( $attributes ) ) {
return $post_content;
}
// Do the magic to parse the html and add the extra attributes (we could start by supporting class and style).
There was a problem hiding this comment.
I think that makes sense.
However, Im not currently seeing any of the similar experimental supports flags for the text alignment and class name. Should we leave these out for now and expect them to be handled elsewhere for the time being (index.php I'm guessing for these blocks)?
There was a problem hiding this comment.
We have the customClassName for the custom classname, classname for the generated classname (which can also be handled similarly) and align for text alligment. I'm fine leaving them for dedicated PRs though and just get the API right in this initial PR.
There was a problem hiding this comment.
I'm fine leaving them for dedicated PRs though and just get the API right in this initial PR.
Sounds good.
We have the customClassName for the custom classname, classname for the generated classname (which can also be handled similarly) and align for text alignment.
I saw these, but the reasons I am confused is that they don't have support keys defined for them like the others (font-size, line-height, colors, etc.) do. Similarly, they aren't checked for in the gutenberg_experimental_global_styles_get_supported_styles function:
gutenberg/lib/global-styles.php
Lines 272 to 278 in b98418d
So I was a bit confused about how they are supposed to be handled compared to the ones we have here that require the support flags, and if they are intended to be added here or elsewhere.
Are we to assume these ones without support keys are supported by default if the corresponding attributes are saved and present? If that is the case we can handle them that way here.
align for text alignment
I was assuming this is for block alignment since it has settings like 'wide' and 'full' width. Or does this combine both text alignment and block/element alignment?
There was a problem hiding this comment.
You're right "align" is for block alignment my bad but it's also a good candidate for server-side generation.
customClassName and className are more "historical" and not part of the global styles project but their behavior is very close. and yes, they are opt-out and I believe className can also receive a string to be used as class (need confirmation)
There was a problem hiding this comment.
i seem something like
Updated to adopt this pattern instead. Let me know if this isn't what you had in mind.
lib/blocks.php
Outdated
| $colors['css_classes'], | ||
| $typography['css_classes'], | ||
| isset( $block['attrs']['className'] ) ? array( $block['attrs']['className'] ) : array(), | ||
| isset( $block['attrs']['align'] ) ? array( 'has-text-align-' . $block['attrs']['align'] ) : array() |
There was a problem hiding this comment.
My feeling is that this function knows too much about what each support flag (align, classnames, experimental ones)... does.
Ideally it's written in a generic way where we'd have a clear "interface" if want to add a new support flag. Doest that make sense?
lib/blocks.php
Outdated
| $colors['css_classes'], | ||
| $typography['css_classes'], | ||
| isset( $block['attrs']['className'] ) ? array( $block['attrs']['className'] ) : array(), | ||
| isset( $block['attrs']['align'] ) ? array( 'has-text-align-' . $block['attrs']['align'] ) : array() |
There was a problem hiding this comment.
i seem something like
$attributes = array();
$attributes = gutenberg_experimental_apply_colors_flag( $attributes, $block['attrs'], $supports );
$attributes = gutenberg_experimental_apply_lineheight_flag( $attributes, $block['attrs'], $supports );
$attributes = gutenberg_experimental_apply-align_flag( $attributes, $block['attrs'], $supports );
$attributes = gutenberg_experimental_apply_custom_classname_flag( $attributes, $block['attrs'], $supports );
if ( ! count( $attributes ) ) {
return $post_content;
}
// Do the magic to parse the html and add the extra attributes (we could start by supporting class and style).
| return $block_content; | ||
| } | ||
|
|
||
| $xpath = new DOMXPath( $dom ); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| * @param array $block Block Object. | ||
| * @return string New block output. | ||
| */ | ||
| function gutenberg_experimental_apply_classnames_and_styles( $block_content, $block ) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
wouldn't testing for the presence of "render_callback" enough maybe?
There was a problem hiding this comment.
That seems to do the trick! Thank you 😄 - 7c17c76
|
Should we at least rebase to get tests to pass and resolve the conflict? (Happy to do it if people don't mind me squashing commits.) |
|
@youknowriad - just double checking to see if you think this is safe to merge now that 8.5 just launched? If so I will circle back and merge Monday morning if no one has beat me to it. |
|
Yep, it should be good to go. |
| $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 ) ) { |
There was a problem hiding this comment.
Thanks for catching it. 👍
| $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 ) ) ); |
There was a problem hiding this comment.
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] =>
)
There was a problem hiding this comment.
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? 🤔
There was a problem hiding this comment.
Do the styles in
$current_stylesnever 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"
}There was a problem hiding this comment.
We can implode
$styles_to_addwith 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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks for pointing this out by the way. I hope to address this relatively shortly but have been preoccupied so far today.
There was a problem hiding this comment.
I proposed a fix in #24486
Part of #21087
Description
Adds block level support for colors, fonts, and line height to the Site Title block.
Adds server side block style support via a function on the
render_blockfilter to apply styles set for all blocks supporting__experimentalColor,__experimentalFontSize, and__experimentalLineHeight.How has this been tested?
Testing instructions
TODO
__experimental*checks before rendering classnames and stylesLook at supporting(This looks too influx to support in this PR).__experimentalPaddingclasses/stylesScreenshots
Types of changes
New feature.
Checklist: