Block Supports: Allow skipping serialization of font size #30879
Conversation
ockham
added
the
Global Styles
|
Size Change: -99.8 kB (-7%) ✅ Total Size: 1.39 MB
ℹ️ View Unchanged
|
ockham
requested review from
ellatrix,
spacedmonkey and
TimothyBJacobs
as code owners
There was a problem hiding this comment.
This tests as advertised and is looking pretty good 👍
I believe we might need an additional check for skipping serialization in withFontSizeInlineStyles in hooks/font-size.js.
This will prevent the inline styles being applied to the block when selecting a different font size from the dropdown or a custom font size that matches a preset.
if (
! hasBlockSupport( blockName, FONT_SIZE_SUPPORT_KEY ) ||
hasBlockSupport(
blockName,
'__experimentalSkipFontSizeSerialization'
)
) {
return <BlockListBlock { ...props } />;
}Regarding the issue of the flat __experimentalSkipFontSizeSerialization flag, it would feel neater if the typography support flags were collected under typography like color or border. As mentioned though that would be a breaking change for themes already using them.
Could an intermediate step be to use a typography.__experimentalSkipSerialization or __experimentalSkipTypographySerialization flag and check that one flag across the typography related block supports?
In other words do we need separate __experimentalSkip...Serialization flags?
There was a problem hiding this comment.
Could an intermediate step be to use a typography.__experimentalSkipSerialization
I like the idea of this intermediate step.
Our block.json supports API right now has inconsistencies in how we handle typography vs colors:
"color": {
"gradients": true,
"link": true
},
"fontSize": true,
"lineHeight": true,
It is also inconsistent with theme.json given that there we have:
"typography": {
"fontSize": "value",
"lineHeight": "value",
}
The intermediate step allows us not to build on these inconsistencies. I guess we can change the shape of block.json and be back-compatible. We can have a back-compatibility layer that checks if things like fontSize and lineHeight are present in the data structure and if yes nests them under typography. Our code excluding the back-compatibility layer would only rely on the new structure.
The shape change would be done in another PR. I'm not sure if a change like this would require a new "apiVersion", I guess it is something minor that does not requires it, but I'm cc'ing @gziolo in case he has some thoughts.
It's already document at https://developer.wordpress.org/block-editor/reference-guides/block-api/block-supports/, but it would be good to validate whether it was released in the previous major version of WordPress. If not then we can simply change it and keep the backward compatibility layer only in the plugin for a few more releases. In general, the question goes to @nosolosw how much Concluding, if this is only a simple mapping layer that would have to map some properties from their deprecated names, then we should definitely go for it. In the documentation, we should simply remove old variants in favor of the refined approach. |
Yay! 🎉 Thanks for reviewing and testing!
I stared at this file for a while when I was working on this PR and decided to leave it as-it. I don't claim I understood it fully at the time (and there's a chance that I still don't 😅 ), but my reading was as follows: The logic in I think it's fine to retain this behavior, even if we're disabling serialization 🤔 The only thing that changes compared to the previous situation is that we're no longer attaching a LMK if this makes sense -- I'm still not 100% sure myself 😅 This comment is a bit cryptic: gutenberg/packages/block-editor/src/hooks/font-size.js Lines 156 to 157 in 928e6a4 typography.fontSizes setting -- but that's what withFontSizeInlineStyles is evaluating anyway 🤔 cc/ @nosolosw @jorgefilipecosta
|
Sounds good to me, too 👍
We already have something like that in the This is getting pretty confusing 😅 All said and done, would we end up with
Should this PR be blocked by the shape change, or can we do that after merging this one? |
Definitely not a blocker. It should be done separately so we could reason about all changes in one place. |
Thanks for the extra background @ockham, I don't claim to be an expert myself either 😄
While it's correct that it's editor specific, the font size styles are serialized for the block's actual markup in the DOM. The markup you see in the code editor view is untouched. So changing the font size selection from the drop down would impact the visual size of the text. If this is being used on a more complicated block where the font size should only be applied to an inner element, while it is in the editor, this rogue style would cause a disjoint between what the user sees in the editor vs frontend.
With the tweak to
A separate PR for the shape change gets my vote as well and shouldn't block this one. |
|
Thanks for taking the time to explain, and to prepare those Gifs, @aaronrobertshaw!
This is where the penny dropped for me. Thanks, I'll apply the change you suggested in #30879 (review) 😊
Glad that you and @gziolo agree! ✨ |
Done per the last couple of commits. Should be ready for another look 😊 |
ockham
force-pushed
the
add/block-supports-skip-serialization-for-font-size
branch
from
bc3ec91 to
581bb3f
Compare
There was a problem hiding this comment.
Thanks for the updates on this 👍
I'd say this one is nearly ready to merge. I just have one comment relating to a difference in flags being checked server-side vs client.
The skipping of font size serialization works well for static blocks. The issue mentioned above however means you'd need a second flag for the same behaviour to occur for a dynamic block rendering via PHP.
Adding the __experimentalSkipFontSizeSerialization flag to the Post Tags block results in the experience below i.e. serialization still occurs for dynamic blocks on the frontend. Bringing the flag checked within typography.php in line fixes this issue.
Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>
|
Thanks @aaronrobertshaw! Really appreciated your review! ✨ |
Description
Part of #28913. Counterpart to #29142+#29253, or #30035.
Find affected blocks by grepping
"fontSize": trueinblock.jsonfiles.How has this been tested?
(Based on @nosolosw's #30035 (comment), and adapted for fontSize block support.)
"__experimentalSkipFontSizeSerialization": trueas a new key-value pair within thesupportsobject for any block that also has"fontSize": trueset. In the following, we'll be using the Heading block.Here's a patch for your convenience:
The expected result is that:
fontSizeattribute, e.g.<!-- wp:heading {"fontSize":"large"} -->-- but not as inlined markup (HTMLclass="has-large-font-size"attribute on the<h2 />tag).typography:{ fontSize: ... } }attribute, e.g.<!-- wp:heading {"style":{"typography":{"fontSize":"25px"}}} -->-- but not as inlined markup (HTMLstyle="font-size:25px"attribute on the<h2 />tag).style="line-height:...attribute on the<h2 />tag).Screencasts
Before
After
Question
fontSizeis a non-experimental, "flat" (non-array) block-supports flag. This is the reason why we're introducing another "flat" flag to skip serialization specifically of font size here:__experimentalSkipFontSizeSerialization.Compare that to "nested" block-supports flags (such as
color), where we can include a "generic"__experimentalSkipSerializationin that array:gutenberg/packages/block-library/src/button/block.json
Lines 56 to 59 in aa272f8
A dedicated
__experimentalSkipFontSizeSerializationflag doesn't scale particularly well, and might get awkward once we allow skipping of serialization for other typographic properties (line height, font family, font appearance, text decoration, text transform). Will each of these get their own__experimentalSkip...Serialization? Or will we have to group typographic block-supports flags into a sharedtypographyobject first? (But that might affect themes that use these already, won't it?) 🤔