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

Latest Comments: Add border block support #66020

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

Latest Comments: Add border block support #66020

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8cf68af
Latest Comments Border Block Support
karthick-murugan Oct 10, 2024
8c2b7e1
Some design issues
karthick-murugan Oct 10, 2024
62bebfb
Removed unwanted styles
karthick-murugan Oct 17, 2024
0b1dd41
Style Update
karthick-murugan Oct 17, 2024
bfe764a
Removed space in css
karthick-murugan Oct 17, 2024
b447807
Some css updates
karthick-murugan Oct 22, 2024
37cb235
Feedback changes updated
karthick-murugan Nov 25, 2024
28e29c6
add the editor.scss file
karthick-murugan Nov 26, 2024
72b3168
Feedback changes updated
karthick-murugan Nov 27, 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
1 change: 1 addition & 0 deletions packages/block-library/src/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
@import "./group/editor.scss";
@import "./html/editor.scss";
@import "./image/editor.scss";
@import "./latest-comments/editor.scss";
@import "./latest-posts/editor.scss";
@import "./media-text/editor.scss";
@import "./more/editor.scss";
Expand Down
12 changes: 12 additions & 0 deletions packages/block-library/src/latest-comments/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,18 @@
},
"interactivity": {
"clientNavigation": true
},
"__experimentalBorder": {
"radius": true,
"color": true,
"width": true,
"style": true,
"__experimentalDefaultControls": {
"radius": true,
"color": true,
"width": true,
"style": true
}
}
},
"editorStyle": "wp-block-latest-comments-editor",
Expand Down
1 change: 1 addition & 0 deletions packages/block-library/src/latest-comments/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ export default function LatestComments( { attributes, setAttributes } ) {
style: {
...attributes?.style,
spacing: undefined,
border: undefined,
},
};

Expand Down
17 changes: 17 additions & 0 deletions packages/block-library/src/latest-comments/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// Higher specificity - target list via wrapper.
.wp-block-latest-comments .wp-block-latest-comments {
// Remove spacing. Higher specificity required to
// override default wp-block layout styles in the Post/Site editor.
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.

There's a difference between the editor and frontend still when it comes to padding.

If no padding is specified in the editor, the frontend still uses the padding-left from the browser's user agent list styles.

We probably need to make that consistent. For backwards compatibility though, I don't think we can just remove the user agent padding. Some themes might be relying on that padding.

I'm not sure if it is possible to make the editor display the user agent list padding-left, without perhaps changing the server side render's outer wrapper to be an ol instead of a div. That would then require massaging all those list styles on the outer wrapper again.

If there's no good option there, perhaps we'll need to get design feedback on whether the user agent styles for the comments block could be safely reset on the frontend.

Copy link
Contributor

@carolinan carolinan Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I would prefer applying the padding: 0 on the front over changing the wrapper, because it is a smaller change.
And the markup in the editor is already pretty complex.

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, my suggestion was only to swap the wrapper from this block's server-side rendering to a list element instead of a div. Then apply any necessary reset styles. I don't think that makes the markup in the editor more complex.

The main goal for me is visual consistency for the user between the editor and frontend. I ultimately don't have a strong opinion on whether that's done by tweaking the block's editor rendering or the additional padding reset on the frontend. I am a bit wary of anything that could be a breaking change for frontend styles though.

As you note it is a small change so perhaps it is something that could be merged early in the release cycle and called out?

// These styles prevent duplicate borders in the Latest Comments block caused by
// server-side rendering injecting additional styles in the editor. They ensure
// consistent appearance in the editor while avoiding conflicts with frontend styles.
border: none;
border-radius: inherit;
}

// Reset margin for inner elements of the server-side rendered block
// to avoid extra spacing introduced in the editor
ol.wp-block-latest-comments {
margin: 0;
}
12 changes: 5 additions & 7 deletions packages/block-library/src/latest-comments/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
.wp-block-latest-comments {
// This block has customizable border, border-box makes that more predictable.
box-sizing: border-box;
}

// Lower specificity - target list element.
ol.wp-block-latest-comments {
// Removes left spacing in Customizer Widgets screen.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this style still be in the frontend styles?

Its specificity is also higher than the style produced by global styles. (0-1-0). The only reason the global styles margins in the editor work is because the block is server-side rendered and the block's outer wrapper receiving the margins is then a div.

In other words, when you moved the margin-left: 0 style to the editor styles. Did you consider removing this? Better yet, did you test removing it at all, especially in the Customizer?

Expand Down Expand Up @@ -26,13 +31,6 @@ ol.wp-block-latest-comments {
}
}

// Higher specificity - target list via wrapper.
.wp-block-latest-comments .wp-block-latest-comments {
// Remove left spacing. Higher specificity required to
// override default wp-block layout styles in the Post/Site editor.
padding-left: 0;
}

.wp-block-latest-comments__comment {
list-style: none;
margin-bottom: 1em;
Expand Down
Loading