Skip to content
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.

Sign up for GitHub

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

RSS: Border & Spacing support #66411

Open
wants to merge 9 commits into
base: trunk
Choose a base branch
from
+42 −10
Open

RSS: Border & Spacing support #66411

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4b56a4a
RSS block border support
benazeerhassan1909 Oct 24, 2024
11696c1
Added block and spacing support for RSS block
benazeer-ben Oct 24, 2024
327b61d
Doc Build
benazeer-ben Oct 24, 2024
bb7cb65
Lint js fix
benazeer-ben Oct 25, 2024
3d05431
Lint js fix
benazeer-ben Oct 25, 2024
209099d
Feed back changes
benazeer-ben Nov 13, 2024
e8c3e09
Review changes
benazeer-ben Nov 13, 2024
4c41f45
Update packages/block-library/src/rss/editor.scss
benazeer-ben Nov 14, 2024
de5a348
Update packages/block-library/src/rss/style.scss
benazeer-ben Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/reference-guides/core-blocks.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -801,7 +801,7 @@ Display entries from any RSS or Atom feed. ([Source](https://github.com/WordPres

- **Name:** core/rss
- **Category:** widgets
- **Supports:** align, interactivity (clientNavigation), ~~html~~
- **Supports:** align, interactivity (clientNavigation), spacing (margin, padding), ~~html~~
- **Attributes:** blockLayout, columns, displayAuthor, displayDate, displayExcerpt, excerptLength, feedURL, itemsToShow

## Search
Expand Down
16 changes: 16 additions & 0 deletions packages/block-library/src/rss/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,22 @@
"html": false,
"interactivity": {
"clientNavigation": true
},
"__experimentalBorder": {
"radius": true,
"color": true,
"width": true,
"style": true,
"__experimentalDefaultControls": {
"radius": true,
"color": true,
"width": true,
"style": true
}
aaronrobertshaw marked this conversation as resolved.
Show resolved Hide resolved
},
"spacing": {
"margin": true,
"padding": true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
},
"editorStyle": "wp-block-rss-editor",
Expand Down
11 changes: 9 additions & 2 deletions packages/block-library/src/rss/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,14 @@ export default function RSSEdit( { attributes, setAttributes } ) {
isActive: blockLayout === 'grid',
},
];

const serverSideAttributes = {
aaronrobertshaw marked this conversation as resolved.
Show resolved Hide resolved
...attributes,
style: {
...attributes?.style,
border: undefined,
spacing: undefined,
},
};
return (
<>
<BlockControls>
Expand Down Expand Up @@ -189,7 +196,7 @@ export default function RSSEdit( { attributes, setAttributes } ) {
<Disabled>
<ServerSideRender
block="core/rss"
attributes={ attributes }
attributes={ serverSideAttributes }
/>
</Disabled>
</div>
Expand Down
7 changes: 7 additions & 0 deletions packages/block-library/src/rss/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

margin: 0;
border: 0;
}
}
Loading