-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tag Cloud: Add color block support #63592
base: trunk
Are you sure you want to change the base?
Conversation
Flaky tests detected in 26f360b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10319485078
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for iterating on this @akasunil 👍
Unfortunately, simply omitting the text
flag from the block.json supports config isn't enough as it is enabled by default when color support is included. It needs to be explicitly turned off.
I've left a suggestion below that has text color disabled for me.
…add/tagcloud-color
As noted over in #63716 (comment), the adoption of color support for this block probably requires following a similar approach to the Latest Comments block. |
My experience with the Latest Comments block was the same. I'm not sure if this is a bug or if this is how block with ServerSideRender suppose to work with block styles. Blog-Home-.-Template-.-gutenberg-.-Editor-.-WordPress.10.webmIssue is basically this, It works perfectly fine with global styles. But when block styles are applied, it doesn't override the global styles, Until we save the template and reload the page. Global styles are applied under css class |
from #63716 (comment)
I agree, that was writing mistake. |
I believe this is a limitation of the server-side rendering for the block. It isn't re-rendered when the block attributes change. This is another reason these server-side rendered blocks require a refactor to JS.
From what you said about the Latest Comments block, the approach there works albeit requiring a reload. Making these server-side rendered blocks consistent is still a win. I don't think it is an option to change block supports from inline styles. P.S. I'll be AFK for couple of weeks in the near future so further reviews might be rather slow. |
Size Change: +156 B (+0.01%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
…add/tagcloud-color
It working fine now. Blog-Home-.-Template-.-gutenberg-.-Editor-.-WordPress.2.webm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've taken this for another spin.
For the most part it tests as advertised but there was one small issue that makes the colors feel a bit clunky when overriding global styles.
Screen.Recording.2024-08-12.at.4.37.06.PM.mp4
The delay in the server side rendering updating means the inner rendering of the block appears slow to update the colors. I notice that behaviour is evident in your recent demo as well.
Similar to how you solved the application of borders to this Tag Cloud block, do you think we could reset the colors to inherit from the outer wrapper and omit the color values from the attributes passed to the server side rendering?
I suspect that would help smooth out the UX a little.
…add/tagcloud-color
It works as expected now. Single-Posts-.-Template-.-gutenberg-.-Editor-.-WordPress.1.webmWe may need to perform the same for the most latest comments block and check all other blocks with serverSideRender. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akasunil apologies for all the twists and turns on this one 🙏
As this has evolved, it seems as we could make the code changes simpler and at the same time make it easier for the block to adopt further block supports in the future.
The creation of serverSideAttributes
is effectively the same as what happens when skipBlockSupportAttributes
is passed as a prop to the server side rendering. If we reinstate that approach and then use the CSS all
property to reset the inner server-side rendered element's styles (preventing duplicate applications of global styles), then we don't have to juggle as many reset styles. That reset would then also work for any other block support style adopted in the future.
Here's a diff with what I'm suggesting
diff --git a/packages/block-library/src/tag-cloud/edit.js b/packages/block-library/src/tag-cloud/edit.js
index dfa00061c2..9a2b531b30 100644
--- a/packages/block-library/src/tag-cloud/edit.js
+++ b/packages/block-library/src/tag-cloud/edit.js
@@ -107,16 +107,6 @@ function TagCloudEdit( { attributes, setAttributes } ) {
setAttributes( updateObj );
};
- // Remove border styles from the server-side attributes to prevent duplicate border.
- const serverSideAttributes = {
- ...attributes,
- style: {
- ...attributes?.style,
- border: undefined,
- color: undefined,
- },
- };
-
const inspectorControls = (
<InspectorControls>
<PanelBody title={ __( 'Settings' ) }>
@@ -196,8 +186,9 @@ function TagCloudEdit( { attributes, setAttributes } ) {
<div { ...useBlockProps() }>
<Disabled>
<ServerSideRender
+ skipBlockSupportAttributes
block="core/tag-cloud"
- attributes={ serverSideAttributes }
+ attributes={ attributes }
/>
</Disabled>
</div>
diff --git a/packages/block-library/src/tag-cloud/editor.scss b/packages/block-library/src/tag-cloud/editor.scss
index 609320205f..be93fc42ab 100644
--- a/packages/block-library/src/tag-cloud/editor.scss
+++ b/packages/block-library/src/tag-cloud/editor.scss
@@ -1,12 +1,9 @@
-// The following styles are to prevent duplicate spacing and border for the tag cloud
-// block in the editor given it uses server side rendering. The specificity
-// must be higher than `0-1-0` to override global styles. Targeting the
-// inner use of the .wp-block-tag-cloud class should minimize impact on
-// other 3rd party styles targeting the block.
+// The following styles prevent duplicate applications of global styles due
+// to the server-side rendered block element also containing the block class.
+// Targeting the inner use of the .wp-block-tag-cloud class should minimize
+// impact on other 3rd party styles targeting the block.
.wp-block-tag-cloud .wp-block-tag-cloud {
+ all: inherit;
margin: 0;
padding: 0;
- border: none;
- border-radius: inherit;
- background: inherit;
}
The main benefit here is that the typography styles will also update immediately when the user customizes them on a block instance so they override global styles. If we don't use the CSS all
property, we could just add all the typography styles.
I don't have a strong opinion on whether using the all
approach is best or if we should be more explicit and verbose. Perhaps a second opinion on that might help move the needle. cc/ @andrewserong
All that aside, functionally, for colors this PR works and is getting close to where we can land it. Thanks for your patience.
What?
Add color block support to the
Tag Cloud
block.Part of #43245
Why?
Tag Cloud
block is missing color support.How?
Adds the color block support in block.json
Testing Instructions
Tag Cloud
block's colors setting are configurable via Global StylesTag Cloud
block and Apply the border stylesScreenshots or screencast
In backend:
In frontend: