Conversation
…s from being printed twice
| style: { | ||
| ...attributes.style, | ||
| spacing: undefined, | ||
| typography: undefined, |
There was a problem hiding this comment.
If border support is added in the future, it will be necessary to write border: undefined here.
There was a problem hiding this comment.
This doesn't filter out the fontSize or fontFamily attributes that are also used by the typography supports to generate CSS classnames. The result is that classes for those are still duplicated which goes against the stated intent of this PR.
As there are a few top-level attributes added by block supports and any block using ServerSideRender needs to remove them along with the style attribute, it seems like we might be able to address this within ServerSideRender itself rather than duplicate that logic across several blocks.
As a quick guide the following top-level attributes added by block supports come to mind;
fontSizefontFamilyborderColorbackgroundColortextColorgradient
There are probably some others as well for things like align support etc.
| @@ -1,3 +1,8 @@ | |||
| .wp-block-archives { | |||
| // This block has customizable padding, border-box makes that more predictable. | |||
| box-sizing: border-box; | |||
There was a problem hiding this comment.
Added according to #43465, but will revert if this fix is not needed now.
|
Size Change: +70 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
aaronrobertshaw
left a comment
There was a problem hiding this comment.
Thank you for putting this PR together @t-hamano 👍
I ran into one issue while testing this. It doesn't remove the duplicated CSS classes as advertised. For typography classes, it would need to remove fontSize and fontFamily top-level attributes.
Taking a step back though, I do wonder if we'd be better served to move this logic of filtering out the block support attributes to the ServerSideRender component itself. It saves us from having to duplicate this same logic across several blocks. What do you think?
I'd also like to touch base with @kevin940726 and @andrewserong, who have either worked on ServerSideRender or looked at issues on this front previously, to get their thoughts.
| style: { | ||
| ...attributes.style, | ||
| spacing: undefined, | ||
| typography: undefined, |
There was a problem hiding this comment.
This doesn't filter out the fontSize or fontFamily attributes that are also used by the typography supports to generate CSS classnames. The result is that classes for those are still duplicated which goes against the stated intent of this PR.
As there are a few top-level attributes added by block supports and any block using ServerSideRender needs to remove them along with the style attribute, it seems like we might be able to address this within ServerSideRender itself rather than duplicate that logic across several blocks.
As a quick guide the following top-level attributes added by block supports come to mind;
fontSizefontFamilyborderColorbackgroundColortextColorgradient
There are probably some others as well for things like align support etc.
Thanks for the ping! From a quick read of the
It also looks like that package hasn't been updated much, so it seems the general idea from other contributors was that we should be updating blocks to not rely on the So I suppose I'd think of the fix in this PR (or an update in From a quick skim, the blocks that currently use
For some of these, I wonder how hard it'd be to do a JS-based version of the server-rendered markup? 🤔 I imagine something like Tag Cloud is likely not really worth the effort to duplicate the PHP, so now that I've written out this comment, I think I know why we're currently using TL;DR: Yes, I think the idea of consolidating the logic of which attributes to skip for block supports styles is a good idea 👍 |
|
Thanks for the extra input @andrewserong 🙇
My view is the blocks you noted are all old existing blocks that began using Also, if we've released the |
|
Thank you for the advice, @aaronrobertshaw, @andrewserong !
I also agree with this opinion. According to WPdirectory's search results, there are several plugins that use As for the core blocks, I think they should be refactored to be as JS-based as possible, but this is a long-term thing, and for now the priority should be to keep If you are comfortable with this direction, I would freeze #44438 and #44439 and consider a new PR to extend ServerSideRender, what do you think? 🙂 |
|
I have added the new |
aaronrobertshaw
left a comment
There was a problem hiding this comment.
Thanks for cleaning this up.
It tests well for me. I could confirm the duplicate classes and styles before checking out this PR. After doing so, the duplicates were omitted.
LGTM 🚢
Follow-up on:
Similar to #44439
Blocker: #44491
Note: I think the double spacing is a major layout issue, so it would be a good idea to be backported to WordPress 6.1.
@edit: As per the discussion that began with this comment, it was decided to move the logic for filtering the block support attributes to the
ServerSideRendercomponent itself. Once the appropriate prop is added to theServerSideRedercomponent, the code changes in this PR will be simpler. This PR will be placed on freeze until a PR regarding improvements toServerSideRenderis submitted and merged.@update: Now that #44491 has been merged, the new
skipBlockSupportAttributesprop of theServerSideRendercomponent is available. This allows us to solve the problem this PR addresses in a simpler way.What?
This PR fixes a problem in the editor where block support inline styles and additional CSS classes are rendered twice.
Why?
This is because this block uses the
ServiceSideRendercomponent and receives all attributes (including styles generated by the block support).How?
As the discussion started with this comment, there are many possible approaches for a block using the
ServiceSideRendercomponent. I have tried to solve this by not passing block support styles and additional classes to theServiceSideRendercomponent.Testing Instructions
ServiceSideRendercomponent and are rendered only in the block wrapper by using the browser console.Screenshots or screencast
Before
After