-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Latest Comments: Add border block support #66020
base: trunk
Are you sure you want to change the base?
Latest Comments: Add border block support #66020
Conversation
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. |
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @karthick-murugan! 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. |
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 your contribution @karthick-murugan
✅ Global Styles apply correctly
✅ Block styles override Global Styles
✅ Styling is consistent between editor and front-end
✅ Box sizing allows parent container to enforce accurate width
✅ Border support is default, consistent with spacing support
Single-Posts-.-Template-.-gutenberg-.-Editor-.-WordPress.webm
LGTM 🎉
@aaronrobertshaw I could appreciate a second opinion to make sure I haven't overlooked anything. |
@aaronrobertshaw - If you have some time, can you please review this PR. Thank you very much in advance. 🙇 |
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 your patience @karthick-murugan this one 👍
This PR is nearly there.
✅ Global styles apply
✅ Block instance styles override global styles
✅ Border controls are displayed by default to match spacing controls
✅ Box sizing allow parent block to enforce width
There are a couple of things that need to be fixed before merging though. I've left comments inline below. They aren't major so shouldn't take too much to get this ready to land.
// The following styles are to prevent duplicate border for the latest comments. | ||
border: none; | ||
border-radius: inherit; |
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'm assuming the intent of these is to prevent duplicate border due to the server side rendering in the editor?
- The inline comment should probably explain why/how there could be duplicate borders
- These are editor only styles, placing them here sends them incorrectly to the frontend too
The padding-left
style above doesn't prevent the duplicate spacing in the editor either. So while you're moving the border resets to an editor.scss
file, padding
needs adding there too.
On that padding note, I'm not sure if the padding-left
rule needs to be on the frontend or not. It might be that the selector here needs lowering in specificity so that global styles can apply correctly.
For this PR, I'd definitely sort out fixing the server-side rendering duplicate styles for padding and border. It's not much extra in terms of scope but if you really want to push the padding-left
thing to a follow-up that's neither here nor there 🙂
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.
Thank you very much for the review, @aaronrobertshaw. I’ve implemented all your feedback and pushed the changes.
The padding-left has been moved to the editor.scss file as it’s not needed on the frontend. Since this resolves the issue, I believe a separate PR for padding-left adjustments won’t be necessary.
Let me know if you have any further suggestions!
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 the iteration @karthick-murugan but my feedback included fixing the duplicate padding caused by the server-side rendering. This isn't addressed by only moving the padding-left
rule.
When fixing this, could you also update the PR test instructions to cover this? Also, how does the relocation of the padding-left
rule affect the issue the PR that introduced the style aimed to solve? Does moving this style cause a regression in older themes e.g. TT1 or TT1 blocks?
In making changes to existing code or styles it's really helpful to keep Chesterton's Fence in mind. The TL;DR on that principle is we need to understand why something is in place before we change or move it to avoid unintended side effects.
On top of that, each time I update a PR, I personally re-run through the test instructions and ask myself questions about what has changed. This helps make sure the PR is easy for others to review and helps reduce the feedback cycle duration, increasing velocity.
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.
Thank you, @aaronrobertshaw, for your detailed feedback and for emphasizing the importance of addressing the root cause and avoiding unintended regressions.
- I have activated the TT1 theme and verified the Latest Comments block. The style .wp-block-latest-comments
.wp-block-latest-comments { padding-left: 0; }
is not applied on the frontend. This confirms that moving the padding-left rule to the editor.scss file does not introduce any regressions for older themes, including TT1 and TT1 Blocks.
REC-20241126132321.1.mp4
-
After moving the padding-left rule to editor.scss, the original intent of removing unnecessary left padding in the editor, customizer, and widgets remains intact. I have recorded a video demonstrating this behavior.
-
Additionally, I have tested the block borders in various scenarios following the recent changes. The video included in the PR reflects the current state, ensuring that the duplicate border issue is resolved without affecting other styles.
REC-20241126131024.mp4
- I have also updated the PR test instructions to explicitly cover these scenarios, helping reviewers validate the changes effectively.
Please let me know if there’s anything else you’d like me to revisit or clarify. I appreciate your guidance and the opportunity to iterate on this.
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 the iteration @karthick-murugan but my feedback included fixing the duplicate padding caused by the server-side rendering. This isn't addressed by only moving the padding-left rule.
This still hasn't been addressed.
Steps to replicate:
- Add latest comments block
- Navigate to Styles > Blocks > Latest Comments
- Add padding
- Note duplicate padding except on the left (because only
padding-left: 0
is in the reset styles.
Screen.Recording.2024-11-27.at.11.06.59.am.mp4
In addition to the padding, the margin on the inner server-side rendered block also needs resetting.
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.
Thank you for your feedback, @aaronrobertshaw. I believe I have addressed the padding and margin issues as part of the recent iteration. To ensure clarity, I’ve recorded a video demonstrating the changes and verifying the behaviour.
REC-20241127124310.mp4
If I’ve misunderstood or overlooked anything, please feel free to correct me. I appreciate your guidance and am happy to make further adjustments as needed. 😊
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.
Appreciate the patience and continued iteration here @karthick-murugan 👍
I've taken this PR for another spin. There are still some easy to encounter issues that need ironing out. They probably also speak to gaps in the test instructions as well.
Margins between the editor and frontend don't match:
Editor | Frontend |
---|---|
![]() |
![]() |
In the above screenshots, I have a Group block, containing 2 paragraphs with the Latest Comments block in between them. The Latest Comments block has a margin applied and it appears as though they are reset correctly in the editor for the server side render, the final margins on the frontend don't match.
Padding doesn't match on frontend
The user agent list styles are present on the frontend but are reset in the editor.
Editor | Frontend |
---|---|
![]() |
![]() |
I've left some comments inline with some extra thoughts.
// 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. |
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.
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?
.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; |
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.
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.
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 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.
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.
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?
The missing Here are the issues: |
I think this is a strong argument for adding the padding reset on the frontend. Bonus points for linking the recent issues highlighting this expectation @carolinan 👍 @jasmussen or @jameskoster if you have a second, would you be against resetting the user agent's |
Assuming you can use low specificity, this seems fine to do. There is an expectation that the frontend matches the backend, so it's an important one to address. |
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 the sanity check @jasmussen 🙇
@karthick-murugan, we should be able to add this padding reset using :where
to keep the specificity to a bare minimum:
:where(.wp-block-latest-comments) {
padding: 0;
}
In addition to that, I think it would pay to look through styles.scss
and editor.scss
to see what duplicate styles have been created by the proposed changes. For example, the new box-sizing
rule appears to double up on the one defined below.
What?
Add border block support to the Latest Comments block.
Part of #43247
Why?
Latest Comments
block is missing border support.How?
Adds the border block support in block.json
Testing Instructions
Latest Comments
block's border is configurable via Global StylesLatest Comments
block and Apply the border stylesTesting Instructions for Keyboard
Screenshots or screencast
In Frontend:
![frontend](https://private-user-images.githubusercontent.com/97787966/375401407-6c2d5977-09d8-43b6-a501-750b2ec57be7.jpg?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzQ2MDA0NjAsIm5iZiI6MTczNDYwMDE2MCwicGF0aCI6Ii85Nzc4Nzk2Ni8zNzU0MDE0MDctNmMyZDU5NzctMDlkOC00M2I2LWE1MDEtNzUwYjJlYzU3YmU3LmpwZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDEyMTklMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQxMjE5VDA5MjI0MFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTJkODM3MjY1ZWRlNjIwNjZiNjc5ZWVkZTJiM2QyZDA5ZjgxODQ5MmRiMmQwZTg2ZjkxYjgyZDkwZjUyNGEwODImWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.a6T1Tk2i82AjydyRud_z5_la7Rcb5FWxF1FKnr-2s0A)
In Backend:
![backend1](https://private-user-images.githubusercontent.com/97787966/375401866-186fd684-1b0a-4777-9b55-936f075814ac.jpg?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzQ2MDA0NjAsIm5iZiI6MTczNDYwMDE2MCwicGF0aCI6Ii85Nzc4Nzk2Ni8zNzU0MDE4NjYtMTg2ZmQ2ODQtMWIwYS00Nzc3LTliNTUtOTM2ZjA3NTgxNGFjLmpwZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDEyMTklMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQxMjE5VDA5MjI0MFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWFhNzliODZlMWQyZGNjMzZiYzJiZDE3M2VkNTc0OTk3ZTk2NGI5Y2VkYzdiYzc3ZGUzODUwNDg5MTgwODRhYTcmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.fyO0gPXS_EevazUwBm53EvDpUoUkjQ1h4E4Slcfhzs4)