ServerSideRender: add new skipBlockSupportAttributes prop
#44491
Conversation
|
Size Change: +162 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
| @@ -2,6 +2,10 @@ | |||
|
|
|||
| ## Unreleased | |||
|
|
|||
| ### Feature | |||
|
|
|||
| - Add `skipBlockSupportAttributes` props to prevent duplication of styles in the block wrapper and the `ServerSideRender` components. [#44491](https://github.com/WordPress/gutenberg/pull/44491) | |||
There was a problem hiding this comment.
If the description is unnatural, please feel free to update it 🙏
t-hamano
changed the title
skipBlockSupportAttributes propskipBlockSupportAttributes prop
t-hamano
added
[Type] Enhancement
There was a problem hiding this comment.
Thanks for your hard work on this @t-hamano 👍
It is testing pretty well for me so far. I was wondering, though, if we might need to add some additional logic to removeBlockSupportAttributes.
A block might wish for some block supports to be serialized on the outer wrapper while it skips other supports so they can be serialized on inner elements. In the latter case, we don't want to remove that support's attributes.
The Calendar block, as you mention, is a prime example of this. It applies typography support classes and styles to the wrapper while skipping serialization of the color support classes and styles so they can be applied internally.
Could we check each block support for __experimentalSkipSerialization within the block type's config? If it is set, don't remove those attributes. It is still possible for a block to skip serialization only to reapply it again on the wrapper. For such an edge case, the block itself would have to handle stripping the attributes itself. That should be pretty uncommon so I think that's fine.
Screenshot for Archives block showing no duplicate classes & styles
|
Thank you for the review, @aaronrobertshaw! The process of removing the block supports was very complicated. For example, in the case of text color, the following two properties need to be deleted:
For borders, it is more complicated. For example,
At the same time, I made the following two changes:
I have tested in a calendar block where A calendar block without skipBlockSupportAttributes
A calendar block with skipBlockSupportAttributes
I believe this new prop alone will solve the other PRs as well 🙂 |
| borderColor: [ '__experimentalBorder', 'color' ], | ||
| }; | ||
|
|
||
| export const STYLE_PROPERTY = { |
There was a problem hiding this comment.
This definition is based on the following:
| * | ||
| * @return {boolean} Whether serialization should occur. | ||
| */ | ||
| function shouldSkipSerialization( blockType, featureSet, feature ) { |
There was a problem hiding this comment.
This function is the same as:
There was a problem hiding this comment.
Amazing effort persevering with the complexities here @t-hamano! 🙇
The process of removing the block supports was very complicated.
I didn't appreciate or maybe I'd repressed memories of how tangled the block supports config and skipping serialization can get 🙈
Knowing the effort you've already put in, I'm a bit reluctant to raise the question of whether this added complexity is worth it here. We might benefit from getting a second opinion on the direction. @andrewserong do you have any thoughts on this PR?
In case we do decide against adding the complexity to this component, you have my apologies for sending you down that path! I don't imagine it was much fun 🙂
I'll outline a few of my concerns below that give me a little cause to rethink our approach.
- There's quite a lot of code being duplicated from other packages here e.g. the constants config,
shouldSkipSerialization,cleanEmptyObjectetc. - I'm not sure people working on block supports would know of or even find the new constants etc here to update.
- This PR introduces further usage of Lodash while there is a concerted effort in Gutenberg to remove our reliance on Lodash so that can be dropped altogether as a dependency.
- It appears that Calendar is the only block using
ServerSideRenderthat skips block support serialization. So it would be the only one benefiting from all the extra complexity added in this PR now.
Would we be better off treating Calendar as a special case and having it not leverage the new skipBlockSupportAttributes prop and instead stripping its own attributes as needed?
What do you think is the best path forward?
|
I don't care about the additional implementation regarding serialization, as I have no problem if this is not incorporated 👍 I'm rethinking my thinking, it might be a rare case for blocks with the |
There was a problem hiding this comment.
Just echoing some of @aaronrobertshaw's thoughts here, but thank you so much @t-hamano for going down this particular rabbithole. I think it's very helpful for revealing some of the trade-offs of the decision making here surrounding both how we use the ServerSideRender component, and where should we put the complexity.
From my perspective, I think the scope of this PR now reveals that we might be better served to deal with the additional duplication in the original PRs that dealt with the block supports problem on a case-by-case basis for each block, in the block's edit components. The reason being that the complexity added here also winds up introducing duplication, that would then need to be maintained as the block supports evolve. Plus, there is also the concern that ideally in the long-term, blocks will reduce their dependency on using the ServerSideRender component altogether.
So, it might wind up being a bit easier for us if we deal with this specifically in the blocks' Edit components as in the original PRs (e.g #44438)?
I very much share @aaronrobertshaw's sentiment that this is great work here, I think it's hugely valuable to explore all our options in depth, and it's very much appreciated! It's always challenging with these sorts of problems where there isn't a single obvious, clean solution, but rather a set of trade-offs to manage complexity and maintainability.
What do you both think?
|
Although only four core/blocks benefit from the change in the ServerSideRender package, there are 620 plugins in the repo who also use it one way or other. Would I be correct, when I write: Before this is merged into Core with 6.2
|
bph
added
the
Needs Dev Note
This new property will keep the existing behavior unless explicitly opted in by the developer. Therefore, I think that the backward compatibility doesn't need to be considered.
Perhaps this might need to be discussed in #45194; whether to stabilize
I am wondering if a dev note should be written on this. This new prop is implemented to solve the problem of double application of block support styles in core blocks using Therefore, it might not be necessary to actively announce it. @aaronrobertshaw |
|
Thanks @t-hamano for entertaining my questions.
Ah interesting, was this formulized in any documentation? 620 plugins can't be that wrong :-) |
|
Please see: https://github.com/WordPress/gutenberg/tree/trunk/packages/server-side-render#serversiderender My understanding is that this package used to be used to render dynamic blocks on the editor. And it had the advantage of being able to use the PHP rendering logic as it is done on the front end. However, nowadays, with the data API, it is possible to implement rendering on the editor side in JavaScript without using this Package. Many developers may continue to use this package because it eliminates the need to write a lot of editor code 🤔 |
Thanks for the ping 👍 I think @t-hamano sums everything up nicely. The only point I would add relates to the stabilization of the For the moment, some other Gutenberg Phase 2 wrap-up tasks are taking priority, but the hope is to address those stabilizations soon. |
Given that Gutenberg is six years old, the group of PHP developer using
That's good to know. So this shouldn't be more than a two paragraph information, as to the why, what, how? |
|
I am wondering if we should put this PR in the dev note, I have two ideas:
@aaronrobertshaw If you don't mind, I'd like to know your opinion 🙏 |
I don't think there is much harm in communicating the change. For those already using |
|
Thank you @aaronrobertshaw! I have written a dev note here. Feel free to edit it. Add new prop to ServerSideRender componentWordPress 6.2 introduces a new Note:
import ServerSideRender from '@wordpress/server-side-render';
const MyServerSideRender = () => (
<ServerSideRender
block="core/archives"
attributes={ attributes }
/>
);If you add block supports to the block that contains this component, the problem is that the styles from the block support will be applied to both the block wrapper element and the rendered HTML by <div
...
class="block-editor-block-list__block wp-block wp-block-archives"
style="padding: 10px; margin: 10px;"
>
<div inert="true" class="components-disabled">
<div>
<ul
style="padding-top:10px;padding-right:10px;padding-bottom:10px;padding-left:10px;margin-top:10px;margin-right:10px;margin-bottom:10px;margin-left:10px;"
class="wp-block-archives-list wp-block-archives"
>
<li><a href="http://example.com">Hello World</a></li>
</ul>
</div>
</div>
</div>By opting in to the new import ServerSideRender from '@wordpress/server-side-render';
const MyServerSideRender = () => (
<ServerSideRender
skipBlockSupportAttributes
block="core/archives"
attributes={ attributes }
/>
);However, if block styles are already defined in the global style or in import ServerSideRender from '@wordpress/server-side-render';
const serverSideAttributes = {
...attributes,
style: {
...attributes?.style,
// Ignore styles related to margin and padding.
spacing: undefined,
},
};
const MyServerSideRender = () => (
<ServerSideRender
// All block support attributes except margins and padding are passed.
attributes={ serverSideAttributes }
block="core/archives"
attributes={ attributes }
/>
); |
t-hamano
added
the
has dev note
|
@t-hamano One thing that might be worth highlighting for the dev note, is that the primary block supports that are negatively impacted by the duplication of classes and styles are the spacing supports (padding, margin). Some blocks have Global Styles generated under their default class e.g. I encountered the above when rebasing the PR adopting typography supports for the Latest Comments block. |
|
Thank you for the advice, @aaronrobertshaw!
I have added the above text and updated the code examples slightly.
My understanding is that such a case occurs when "__experimentalSelector" or |
This isn't the case, unfortunately. Using #43310 as an example. It doesn't define any custom selector and doesn't skip serialization. If you set global styles for the Latest Comments block's typography, then switch to the editor and try to change these typography styles for a single Latest Comments block, they wouldn't be applied in the editor unless we still allow those individual block typography styles to be applied to the ServerSideRendered markup. It's this fact that led me to need to remove the Given that example doesn't relate to experimental APIs such as |
|
Okay, I understand.
I have added the above instructions and a new code example. Feel free to update if there is anything that is not clear 🙏 |
I'll try and clarify this further, still using the latest comments block example; In the editor, the block's wrapper gets the
If you set, say font family, on the block in Global Styles, the global styles stylesheet gets the following: If you then apply a new font family on an individual block in the post editor, that gets applied to the block's outer wrapper. If you also pass I hope that makes things a little clearer 🤞 |
|
Thanks for the detailed explanation, @aaronrobertshaw! I think I have understood it well enough 🙏 |
|
@t-hamano Thanks for going through the process and finalizing the Dev Note. Would you please add this to the Make blog as draft so we can have it reviewed for publishing? Please let me know if you need author access to the site. |
bph
removed
the
Needs Dev Note
|
I have written a dev note draft: https://make.wordpress.org/core/?p=102933 |
|
Hi! 👋 I'm catching up on this PR after reading the dev note. One thing that struck me is that this setup with Even as we speak, there are discrepancies between that function and the actual map of support attributes: # List all attributes names corresponding to block-supports features:
$ grep -Eho "attributes\['\w+'\]" lib/block-supports/*.php | awk -F\' '{print $2}' | sort | uniq
anchor
backgroundColor
borderColor
class
fontFamily
fontSize
gradient
layout
shadow
style
textColorIf we treat |
|
@mcsf We discussed in this PR whether we should finally introduce the As you say, when new block support is added, its attributes and styles should be added in the Maintenance would occur either way, but after consideration, I made a decision that introducing this prop would be less costly to maintain. As far as I can see, the new properties that should be added as exclusions are the In any case, we may want to explore whether there is a way to detect when new block support is added in the future and update this package automatically or manually. |
|
As far as I know, the I added the To prevent this, it is generally necessary to control this with the
|
Related to:
For more information, see the discussion that begins with this comment.
What?
This PR adds a new
skipBlockSupportAttributesprop to theServerSideRendercomponent. If this prop is specified, theServerSideRendercomponent will not include attributes and styles applied by the block support in the API request data.This will make it easier to properly add support to blocks with
ServerSideRendercomponents.Why?
When styles are applied with block support, attributes such as
attributes.textColorandattribites.style.border.radiusare added. If these are passed to the callback function via theServerSideRendercomponent,get_block_wrapper_attributeswill generate the styles and CSS classes. This is the correct behavior on the front end, but on the editor, both the block wrapper and the rendered element will have duplicate styles and CSS classes.This problem can be solved by properly excluding properties and styles added by block support from the attributes passed to the
ServerSideRendercomponent. However, it would make sense for components to include the logic and provide them as props.How?
Remove all properties and styles automatically added by the block supports from the attributes passed to the
ServerSideRendercomponent. This ensures that the block support styles are properly applied only to the block wrapper element on the editor.Testing Instructions
core/archivescore/latest-commentscore/rsscore/tag-cloudblock.json, opt-in all block support as follows:block.json support properties
{ "supports": { "align": true, "html": false, "color": { "text": true, "background": true, "gradients": true, "link": true }, "typography": { "fontSize": true, "lineHeight": true, "__experimentalFontFamily": true, "__experimentalFontWeight": true, "__experimentalFontStyle": true, "__experimentalTextTransform": true, "__experimentalTextDecoration": true, "__experimentalLetterSpacing": true }, "spacing": { "margin": true, "padding": true }, "__experimentalBorder": { "radius": true, "color": true, "width": true, "style": true } } }skipBlockSupportAttributesprop as follows:Screenshots or screencast
Before
After
Next step
This PR does not fix the duplicate custom CSS class that #44439, #44438, and #43653 are trying to resolve. Because I think duplicate CSS classes should be fixed regardless of this new prop. Once this PR is properly merged, I would like to submit a new PR to resolve the CSS class duplication.
Once the CSS class duplication is resolved and the skipBlockSupportAttributes are available, the following PR should be solved with simpler code:
However, for calendar blocks, this option would not work because color support is need to be applied to the internal elements, as was done in #42029.