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
Open

Latest Comments: Add border block support #66020

Show file tree
Hide file tree
Changes from 6 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
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
8 changes: 8 additions & 0 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 @@ -31,6 +36,9 @@ ol.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;
// The following styles are to prevent duplicate border for the latest comments.
border: none;
border-radius: inherit;
Copy link
Contributor

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 🙂

Copy link
Contributor Author

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!

Copy link
Contributor

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.

Copy link
Contributor Author

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.

  1. 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

  1. 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.

  2. 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
  1. 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.

Copy link
Contributor

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:

  1. Add latest comments block
  2. Navigate to Styles > Blocks > Latest Comments
  3. Add padding
  4. 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.

Copy link
Contributor Author

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. 😊

}

.wp-block-latest-comments__comment {
Expand Down
Loading