Conversation
…ses from being printed twice
t-hamano
commented
Sep 25, 2022
| className: undefined, | ||
| style: { | ||
| ...attributes.style, | ||
| spacing: undefined, |
Contributor
Author
There was a problem hiding this comment.
If border support is added in the future, it will be necessary to write border: undefined here.
Contributor
There was a problem hiding this comment.
There is an open PR adding typography supports to the Tag Cloud block. This would need updating here, in that PR, or immediately after it lands.
This is probably further justification that we should move the logic for stripping block support related attributes for <ServerSideRender> as suggested over on the PR doing this for the Archive block.
t-hamano
commented
Sep 25, 2022
| @@ -0,0 +1,4 @@ | |||
| .wp-block-tag-cloud { | |||
| // This block has customizable padding, border-box makes that more predictable. | |||
| box-sizing: border-box; | |||
Contributor
Author
There was a problem hiding this comment.
Added according to #43465, but will revert if this fix is not needed now.
|
Size Change: +147 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
Contributor
aaronrobertshaw
left a comment
There was a problem hiding this comment.
Thanks for working on cleaning up these styles and classes @t-hamano 👍
This is testing well for me.
However, as per my review on these changes for the Archive block, I think we'd be better served moving the logic for stripping block support related attributes to a single location rather than duplicating it across several blocks.
There also look to be a couple of minor copy and pasted typos 🙂
| className: undefined, | ||
| style: { | ||
| ...attributes.style, | ||
| spacing: undefined, |
Contributor
There was a problem hiding this comment.
There is an open PR adding typography supports to the Tag Cloud block. This would need updating here, in that PR, or immediately after it lands.
This is probably further justification that we should move the logic for stripping block support related attributes for <ServerSideRender> as suggested over on the PR doing this for the Archive block.
Contributor
Author
|
Thank you for the review, @aaronrobertshaw !
Indeed, it would be better to first set a policy in #44438 and then work on this PR again. |
t-hamano
commented
Oct 7, 2022
| { inspectorControls } | ||
| <div { ...useBlockProps() }> | ||
| <ServerSideRender | ||
| key="tag-cloud" |
Contributor
Author
There was a problem hiding this comment.
I found that only this block has a key prop among the blocks using the ServerSideRender component.
But since the key prop is not accepted by the ServerSideRender component, I removed it.
Contributor
Author
|
I have added the new |
Contributor
aaronrobertshaw
left a comment
There was a problem hiding this comment.
Thanks for the work behind getting this PR ready @t-hamano.
In my testing, this does resolve the original issue of duplicate classes and styles being applied to this block. However, it only applies the box-sizing reset within the editor. We should be keeping the editor and frontend consistent where possible.
Can we move the box-sizing: border-box to the style.scss file instead?
Contributor
Author
I have taken this action and confirm that it continues to work as expected. |
This was referenced Mar 21, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up on #43367
Similar to #44438
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