Block Supports: Allow skipping serialization of individual features #36293
Conversation
aaronrobertshaw
added
[Feature] Block API
|
Size Change: +190 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
aaronrobertshaw
force-pushed
the
try/skip-individual-feature-serialization
branch
2 times, most recently
from
d7d4d81
to
fc2cb54
Compare
aaronrobertshaw
force-pushed
the
try/skip-individual-feature-serialization
branch
from
fc2cb54
to
cffa9f6
Compare
aaronrobertshaw
changed the title
aaronrobertshaw
requested review from
ellatrix,
spacedmonkey and
TimothyBJacobs
as code owners
aaronrobertshaw
force-pushed
the
try/skip-individual-feature-serialization
branch
from
cffa9f6
to
36e7109
Compare
aaronrobertshaw
force-pushed
the
try/skip-individual-feature-serialization
branch
from
36e7109
to
9865b77
Compare
aaronrobertshaw
force-pushed
the
try/skip-individual-feature-serialization
branch
from
9865b77
to
9d957b0
Compare
aaronrobertshaw
force-pushed
the
try/skip-individual-feature-serialization
branch
from
9d957b0
to
f192859
Compare
|
This is working well for me so far. I've been testing using the Group block. Skipping serialization for individual properties, e.g., "__experimentalBorder": {
"color": true,
"radius": true,
"style": true,
"width": true,
"__experimentalSkipSerialization": [ "style" ],
...isolates that property, and it's no longer available in the block's An empty array – I've rebased to kick off the e2es again, and will keep testing the individual properties. 🍺 |
ramonjd
force-pushed
the
try/skip-individual-feature-serialization
branch
from
f192859
to
bf5ca6c
Compare
|
We were getting a PHP warning – – because 5d99fb7 checks |
|
I think Edit: 7bdb411 addresses this by ensuring that the |
ramonjd
force-pushed
the
try/skip-individual-feature-serialization
branch
2 times, most recently
from
62518a6
to
7bdb411
Compare
|
The fixture failures are curious. They started failing after 7bdb411. Reverting the commit locally via GIT makes them pass again. But also locally, when I roll back the changes individually in each file (basically reverting manually), the tests still fail. The failures themselves are related to the order of block attributes only. 🤔 |
@ramonjd just taking a look at the commit where the fixture tests started to fail. In that change, One thing to try might be to move |
ramonjd
force-pushed
the
try/skip-individual-feature-serialization
branch
from
af420ab
to
4bf4fd7
Compare
| @@ -18,6 +18,13 @@ function gutenberg_render_elements_support( $block_content, $block ) { | |||
| return $block_content; | |||
| } | |||
|
|
|||
| $block_type = WP_Block_Type_Registry::get_instance()->get_registered( $block['blockName'] ); | |||
| $skip_link_color_serialization = gutenberg_should_skip_block_supports_serialization( $block_type, 'color', 'link' ); | |||
There was a problem hiding this comment.
Just a thought: I'm wondering what this might look like if elements is made generic as mentioned in #31524. If we can use it with any element in any style, perhaps skipping serialization will be redundant?
There was a problem hiding this comment.
If we can use it with any element in any style, perhaps skipping serialization will be redundant?
That's a good question, and one I can't answer, though from a superficial reading of the supports file and the conversation of the PR you linked to, I'd probably side with you on that assumption. 😄
I'm not sure there are any plans slated for the future of the elements mechanism 🤔
There was a problem hiding this comment.
I'm not sure there are any plans slated for the future of the elements mechanism 🤔
Right now we are artificially limiting to some elements like links but the mechanism is generic and we can expand it. I'm not sure if someone is working on that.
I agree ideally the elements mechanism would remove the need to skip serialization but I guess until that mechanism is expanded we can have experimental flags to skip serializtion and then we check if we can use elements for these cases.
| @@ -65,7 +66,7 @@ function gutenberg_get_layout_style( $selector, $layout, $has_block_gap_support | |||
| $style .= "$selector .alignleft { float: left; margin-right: 2em; margin-left: 0; }"; | |||
| $style .= "$selector .alignright { float: right; margin-left: 2em; margin-right: 0; }"; | |||
| if ( $has_block_gap_support ) { | |||
| $gap_style = $gap_value ? $gap_value : 'var( --wp--style--block-gap )'; | |||
| $gap_style = $gap_value && ! $should_skip_gap_serialization ? $gap_value : 'var( --wp--style--block-gap )'; | |||
There was a problem hiding this comment.
If we're skipping serialization would we still expect there to be an output on the front end, even if it's a default value?
There was a problem hiding this comment.
Yeah, this is another tricky one. 🤣
I agree in principle, and it did cross my mind, however there's a consideration that caused me to hesitate: --wp--style--block-gap's value comes from theme.json.
So my reasoning at the time was, since skipSerialization acts at the block level, and is indeed meant to offer finer-grained control of block supports, it probably shouldn't override the theme's settings.
Skipping serialization, as far as I understand it, skips user-facing block customization, that is the editor styles controls.
Aside from all that, blockGap is another one of those anomalies. It straddles block supports and layout, and doesn't make life easy when working with both.
Hopefully we can consolidate/homogenize the use of layout and blockGap in the effort to make styles and layouts consistent. See #39336
There was a problem hiding this comment.
Yeah this is a tough one. On the whole I'm more inclined to leave it out because the theme only defines the variable value, not where to use it, and I'd expect skipSerialization to not output anything. But I think we can leave it as is and revisit later; outputting a couple of default styles shouldn't be a huge deal.
| * @param array $layout Layout object. The one that is passed has already checked the existence of default block layout. | ||
| * @param boolean $has_block_gap_support Whether the theme has support for the block gap. | ||
| * @param string $gap_value The block gap value to apply. | ||
| * @param boolean $should_skip_gap_serialization Whether to skip applying the user-defined value set in the editor. |
There was a problem hiding this comment.
Should we not also allow skipping serialization for the whole layout feature too? The Navigation block already implements custom logic to apply the layout styles to its inner elements and blocks, so there's definitely a use case.
There was a problem hiding this comment.
Should we not also allow skipping serialization for the whole layout feature too?
Good point! Thanks for mentioning it. I think this would be a good follow up PR.
I'm guessing @aaronrobertshaw started the work here with a view of extending block supports (border, spacing et. al) that already use skipSerialization in various block.jsons today.
The layout implementation, being a little "different" might also need a level of testing that might be more suited to a dedicated PR. 😄
There was a problem hiding this comment.
Yup, layout is complex enough that a follow-up PR sounds right.
There was a problem hiding this comment.
I'm only just getting up to speed after some recent AFK so might be missing some context.
When originally creating this I found a few block supports that simply didn't maintain/add the ability to skip serialization as per other block supports. Layout was one example of this. It also changed several times during the long life span of this PR. Early on, I had a skip for the entire layout feature but the support was reworked which, from memory, removed the code where that could easily be performed.
I think a separate PR would definitely be a good idea given how complicated layout support is and how much it's already changed in its relatively short life.
Thanks @tellthemachines I really appreciate the 👀 I've commandeered this while @aaronrobertshaw takes a well-deserved rest, so I might not have all the context on those questions. The linked PR #36104, which depends on and is the motivation for the changes in this PR, provides an example of the initial use case. |
…sometimes does not pass a valid $block_type, which must be an object. It is `NULL`. Perhaps due to a race condition. This commit checks if $block_type is an object before trying to use it.
… [ "blockGap" ]` has an effect on the frontend and in the editor. The blockGap value, where defined by the user, will not be applied. Added a comment clarifying the use of skip serialization in relation to blockGap
… importing from style.js
Adding skip serialization tests
…ents API) Added a dev comment to warn about future changes (if any) to the Elements API
aaronrobertshaw
force-pushed
the
try/skip-individual-feature-serialization
branch
from
fa4a95c
to
aa0e91d
Compare
|
I've rebased this and fixed some whitespace errors in the spacing block support PHP unit tests. ✅ Unit tests are passing Primarily used the Group and Paragraph blocks for testing static blocks opting into any missing supports as needed. Used the Site Title block for testing dynamic blocks. I believe this is looking good but it would be great to get an extra review and approval. |
Related:
Description
This PR allows blocks to skip the serialization of individual features within a block support toolset. For example, only skipping the serialization of
text-decorationstyles for the typography support.Updates include:
addSavePropsfilter from thestyle.jshookHow has this been tested?
npm run test-unit packages/block-editor/src/hooks/test/style.js__experimentalSkipSerializationflag still works__experimentalSkipSerializationTesting Summary
You can use
group/block.jsonas the basis for tests. Test using both the boolean –"__experimentalSkipSerialization": true– and the individual features, e.g.,Individual skip serialization example for Group block.json
Group Block (static block)
Buttons Block (static block)
Paragraph Block (static block)
The following block supports were manually added for testing purposes.
Site Title ( dynamic block )
The following block supports were manually added for testing purposes.
Navigation ( dynamic block )
Types of changes
Enhancement.
Checklist:
*.native.jsfiles for terms that need renaming or removal).