RSS: Border & Spacing support #66411

benazeer-ben · 2024-10-24T12:04:41Z

What?

Adding border support & spacing support for RSS block.

Part of #43247 and #43243

Why?

RSS block is missing border & spacing support

How?

Adds the border & spacing support via block.json.

Testing Instructions

Go to Global Styles setting ( under appearance > editor > styles > edit styles > blocks ).
Make sure that RSS block's border is configurable via Global Styles.
Verify that Global Styles are applied correctly in the editor and frontend.
Edit template/page, Add Page List block and apply the border, spacing( padding & margin) styles.
Verify that block styles take precedence over global styles.
Verify that block styles display correctly in both the editor and frontend.

Screenshots or screencast

Backend:

Frontend:

github-actions · 2024-10-24T12:04:53Z

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 props-bot label.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @benazeerhassan1909.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Unlinked contributors: benazeerhassan1909.

Co-authored-by: benazeer-ben <[email protected]>
Co-authored-by: ramonjd <[email protected]>
Co-authored-by: aaronrobertshaw <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

github-actions · 2024-10-24T12:05:22Z

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @benazeer-ben! In case you missed it, we'd love to have you join us in our Slack community.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

ramonjd

Thank you again for helping move these block supports PRs forward 🙇🏻

✅ Block styles are working well.

We just need to ensure padding overwrites the block's styles for global + theme.json styles.

packages/block-library/src/rss/edit.js

ramonjd · 2024-11-11T03:25:37Z

packages/block-library/src/rss/style.scss

@@ -41,3 +41,10 @@ ul.wp-block-rss { // The ul is needed for specificity to override the reset styl
 	display: block;
 	font-size: 0.8125em;
 }
+.wp-block-rss {
+	box-sizing: border-box;
+	.wp-block-rss {


Same here, a comment might help future contributors to know what this does.

I believe these styles are to prevent the double-up of block support styles within the editor when server-side rendering, hence the nested .wp-block-rss classes.

Given they are editor-only styles, they shouldn't be added to the styles sent to the frontend, instead editor.scss might be a better home for them.

These styles are missing the padding reset and the border reset might be too simplistic when Global Styles are taken into account. The tag cloud block went through these same edge cases so it's a good example to follow for the style resets.

My memory failed me during my review but finally kicked into gear. There's a slightly cleaner way to wrangle resetting all the various block support styles when skipping block supports in the server-side rendering. It should also be more forward compatible for future block supports.

It was first proposed for the Tag Cloud block that runs into similar issues.

.wp-block-rss .wp-block-rss { all: inherit; /* This addresses borders, background colors etc. */ margin: 0; padding: 0; }

Moved the nested style in editor.scss as per the discussion, since it is editor only style.

One scenario, while we adding all: inherit; list style get inherited and bullets will display in the editor part.

So in that case, just updated the style as follows:

.wp-block-rss .wp-block-rss { margin: 0; padding: 0; border: 0; border-radius: inherit; background: inherit; }

Please share your thought on this @aaronrobertshaw

Please share your thought on this @aaronrobertshaw

I think we should still use the all: inherit; rule.

The issue with the list-style: none rule not being applied is just that the main style adding it targets the ul element. When the block is server-side rendered, it's wrapper becomes a div.

The fix is as simple as moving the existing list-style: none; rule to the new style you've added using the straightforward 0-1-0 specificity .wp-block-rss selector. Along with that move, we can also clean up the unneeded &.is-grid style's rule for list style as well.

The reset.scss styles are scoped with :where(.editor-styles-wrapper) nowadays so the block doesn't need the bumped specificity. Once this PR is updated though we'll need to double check a non-iframed editor and ensure everything still works as expected.

The benefit of the all: inherit rule is that it is more robust in the face of changes than arbitrarily adding all possible resets. As the list of CSS properties that need to be reset grows, it also means more CSS. The all: inherit; approach keeps that to a minimum.

ramonjd · 2024-11-11T03:39:13Z

packages/block-library/src/rss/block.json

+		},
+		"spacing": {
+			"margin": true,
+			"padding": true


Theme.json and user global styles are being overwritten on the frontend by the RSS block's styles:

gutenberg/packages/block-library/src/rss/style.scss

Lines 2 to 3 in 11696c1

list-style: none;

padding: 0;

It's there to remove the default list padding.

Here's the PR that introduced it: #33294

Reducing specificity might work:

:where(ul.wp-block-rss) { padding: 0; }

For clarity, I believe the suggestion here is to extract the padding to a new style with the reduced specificity, as shown above, rather than change the selector of the entire block of styles.

Exactly, thanks for translating my fly-by comment 👍🏻 😄

Extracted the padding to a new style in the recent commit.

Unfortunately, the original padding style wasn't removed so the problem still exists on the frontend.

aaronrobertshaw

Thanks for your work here @benazeer-ben, appreciate it 👍

In addition to Ramon's feedback, I've spotted a few other tweaks we should make while iterating here.

The last direction I had was that all the blocks that needed spacing support as a "primary use-case" had already had it added. New blocks adopting the support probably shouldn't have the spacing controls shown by default.
- This would be good to confirm again with our design and product experts though.
If we err on the side of caution and don't make the spacing controls display by default, the border controls should match.
- For now, I'd make both sets of controls optional (hidden by default). It's easier to expose them later than hide them which might confuse users that have grown use to them shown by default.
The server-side rendering style resets should be in the editor-only stylesheet.
The server-side rendering style resets are missing padding styles and the border resets might not be complete.

All that said, this is coming along nicely, great work so far ✨

packages/block-library/src/rss/block.json

aaronrobertshaw · 2024-11-11T07:07:19Z

packages/block-library/src/rss/block.json

+		},
+		"spacing": {
+			"margin": true,
+			"padding": true


For clarity, I believe the suggestion here is to extract the padding to a new style with the reduced specificity, as shown above, rather than change the selector of the entire block of styles.

aaronrobertshaw · 2024-11-11T07:09:01Z

packages/block-library/src/rss/style.scss

@@ -41,3 +41,10 @@ ul.wp-block-rss { // The ul is needed for specificity to override the reset styl
 	display: block;
 	font-size: 0.8125em;
 }
+.wp-block-rss {
+	box-sizing: border-box;
+	.wp-block-rss {


I believe these styles are to prevent the double-up of block support styles within the editor when server-side rendering, hence the nested .wp-block-rss classes.

Given they are editor-only styles, they shouldn't be added to the styles sent to the frontend, instead editor.scss might be a better home for them.

These styles are missing the padding reset and the border reset might be too simplistic when Global Styles are taken into account. The tag cloud block went through these same edge cases so it's a good example to follow for the style resets.

aaronrobertshaw

Thank you for iterating here @benazeer-ben 👍

There's a few more tweaks needed before we can merge this one.

The spacing controls still need to be made optional i.e. not displayed by default.
- It's easier to make something show by default later than hide them when users might have already grown accustomed to their location.
The padding: 0 under ul.wp-block-rss wasn't removed so the original issue Ramon flagged is still there when checking the frontend, despite the new style.
- Global Styles also only requires a specificity of 0-1-0 or lower for the block library styles. The padding: 0; rule can live under the .wp-block-rss selector.
- I don't think the comment stating the padding rule was extracted is needed.
There's now in effect a duplicate box-sizing: border-box rule. We should only need one.
- The reset.scss styles are now scoped by :where(.editor-styles-wrapper) so the originalbox-sizing rule's specificity bump isn't required.
- We should remove it in favour of the new rule you added under the simple .wp-block-rss selector.
- The comment relating to box-sizing can then be moved above the rule it relates to.
- A lot of blocks set the box-sizing: border-box rule and use the following comment. It might be best to reuse that comment too so that when a generic solution lands, it's easier to make sure this gets cleaned up as well.
  - // This block has customizable padding, border-box makes that more predictable.
The addition of the background: inherit reset style should probably only happen in the PR that adds color support. Keep this PR's scope only to the spacing and border supports.
- Better yet, we should use the all: inherit; approach, suggested in my last review.
- The list-style issue you flagged can be solved by moving the list-style: none styles to the .wp-block-rss style (note the lack of ul in that selector).
- While on the list-style rules, the one associated with the grid layout for the RSS block doesn't look to be needed.
Nit: The multiline comment in edit.js should probably start with /* instead of /** as it isn't a docblock comment. At least I think that's the reasoning behind the style guideline.

Sorry for the wall of text but I thought it might be easier to tick off all that's left if you had a single list to work from rather than fragmented comments left inline.

Despite my long-winded review, I don't think these tweaks will take much to achieve.

I hope that helps! 🙏

benazeer-ben · 2024-11-13T08:58:47Z

Thanks again for the detailed review on this @aaronrobertshaw , it helped to figure out the scenarios easily.

I have worked on the points as per the above explanation. If it still miss something or misunderstood, kindly let me know, will look into that.

aaronrobertshaw

Thanks for iterating again @benazeer-ben 🚀

There were a couple more minor things missed. I've added suggested tweaks below.

packages/block-library/src/rss/editor.scss

packages/block-library/src/rss/style.scss

Co-authored-by: Aaron Robertshaw <[email protected]>

benazeerhassan1909 and others added 2 commits October 24, 2024 12:20

RSS block border support

4b56a4a

Added block and spacing support for RSS block

11696c1

benazeer-ben requested a review from ajitbohra as a code owner October 24, 2024 12:04

github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Oct 24, 2024

akasunil added [Type] Enhancement A suggestion for improvement. [Block] RSS Affects the RSS Block - used to display entries from an RSS/Atom feed labels Oct 24, 2024

This was referenced Oct 24, 2024

Tracking: Border Design Tools Consistency #43247

Open

Tracking: Dimensions Design Tools Consistency #43243

Open

Doc Build

327b61d

benazeer-ben requested review from juanmaguitar, fabiankaegy and ndiego as code owners October 24, 2024 17:02

benazeer-ben added 2 commits October 25, 2024 11:07

Lint js fix

bb7cb65

Lint js fix

3d05431

ramonjd mentioned this pull request Nov 11, 2024

RSS: Added Colour support #66419

Open

ramonjd reviewed Nov 11, 2024

View reviewed changes

aaronrobertshaw reviewed Nov 11, 2024

View reviewed changes

Feed back changes

209099d

aaronrobertshaw reviewed Nov 13, 2024

View reviewed changes

Review changes

e8c3e09

aaronrobertshaw reviewed Nov 14, 2024

View reviewed changes

packages/block-library/src/rss/editor.scss Outdated Show resolved Hide resolved

packages/block-library/src/rss/style.scss Outdated Show resolved Hide resolved

benazeer-ben and others added 2 commits November 14, 2024 10:30

Update packages/block-library/src/rss/editor.scss

4c41f45

Co-authored-by: Aaron Robertshaw <[email protected]>

Update packages/block-library/src/rss/style.scss

de5a348

Co-authored-by: Aaron Robertshaw <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RSS: Border & Spacing support #66411

RSS: Border & Spacing support #66411

benazeer-ben commented Oct 24, 2024

github-actions bot commented Oct 24, 2024 •

edited

Loading

github-actions bot commented Oct 24, 2024

ramonjd left a comment

ramonjd Nov 11, 2024

aaronrobertshaw Nov 11, 2024

aaronrobertshaw Nov 11, 2024

benazeer-ben Nov 13, 2024

aaronrobertshaw Nov 13, 2024

ramonjd Nov 11, 2024

aaronrobertshaw Nov 11, 2024

ramonjd Nov 11, 2024

benazeer-ben Nov 13, 2024

aaronrobertshaw Nov 13, 2024

aaronrobertshaw left a comment

aaronrobertshaw Nov 11, 2024

aaronrobertshaw Nov 11, 2024

aaronrobertshaw left a comment

benazeer-ben commented Nov 13, 2024

aaronrobertshaw left a comment

RSS: Border & Spacing support #66411

Are you sure you want to change the base?

RSS: Border & Spacing support #66411

Conversation

benazeer-ben commented Oct 24, 2024

What?

Why?

How?

Testing Instructions

Screenshots or screencast

github-actions bot commented Oct 24, 2024 • edited Loading

Unlinked Accounts

github-actions bot commented Oct 24, 2024

ramonjd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronrobertshaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronrobertshaw left a comment

Choose a reason for hiding this comment

benazeer-ben commented Nov 13, 2024

aaronrobertshaw left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 24, 2024 •

edited

Loading