-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Comment Date: Add Border Support #64210
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. |
Size Change: +166 B (+0.01%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
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 PR!
My guess is that it might be better to apply the border to the time
element rather than the block wrapper. Otherwise we might end up with a border that is wider than the actual text.
The screenshot below shows a border applied to the Comment Date block in TT4.
I'd love to hear @aaronrobertshaw's opinion as well.
Thank you for the feedback! Are you suggesting that the border should be applied directly to the I’ve added border support to the |
Thanks for the ping. @t-hamano can you provide some additional steps to replicate the issue when the comment date wrapper stretches wider than the inner
This is a good question. Initially, I was in two minds while reading through this conversation but think I've settled on leaning towards the styles being applied to the outer wrapper. My primary reason is that pre-existing block supports are already applied to the wrapper. This includes color and padding support. If the background color is set on the wrapper and that stretches wider than the inner element it would be odd for the border to not wrap around everything with the background color. As @shail-mehta noted, this does match the recent addition of the border for the Post Date block. The addition of border support to the Post Date block was only made recently though so it could be possible to relocate those styles if it is decided they are better on the inner element. We might even be able to avoid a deprecation as both are dynamic blocks. Finally, the |
Thanks for the feedback!
You should be able to reproduce this by doing the following in this branch:
56ad9c7ba5988b9b40b218f914bb0b6b.mp4
I agree with this. I think all block supports should target the same object unless there's a specific reason not to. I know there are a lot of PRs being submitted to add border support right now. One new thing I noticed here is that there might be cases where users want to achieve a badge-like style: If we want to achieve this style via the global styles UI, we need to target the If we apply all block supports to the wrapper, this style cannot be achieved via the global styles UI. Users would need to define Of course, to achieve this style, the existing background color and padding support must be changed to target inner elements. Also, this change may cause backwards compatibility problems. What do you think about covering cases like this? |
Ah I finally get it, that's right. As the screenshot below shows, if the comment author name is wider than the comment date, the border of the comment date will be wider than the actual text: I think this problem is because the comment author name and comment date are wrapped in a Group block. Changing to a stacked layout would solve it, so this probably needs to be fixed on the TT4 side. Anyway, all things considered, it might be best to apply border support to the wrapper 👍 |
I'm on board with that approach 👍
Agreed. I'll try and give this one a final review shortly. |
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 on this one @shail-mehta 👍
✅ Global Styles apply correctly to the block
✅ Block instance styles override Global Styles correctly
✅ Frontend and editors match
✅ Box sizing allows parent block to enforce width
LGTM 🚢
Screen.Recording.2024-08-09.at.1.51.15.PM.mp4
Co-authored-by: shail-mehta <[email protected]> Co-authored-by: t-hamano <[email protected]> Co-authored-by: aaronrobertshaw <[email protected]>
What?
Add border block support to the
Comment Date
block.Part of #43247
Why?
Comment Date
block is missing border support.How?
Adds the border block support in block.json.
Testing Instructions
Comment Date
block's border is Configurable via Global Styles.Comment Date
block and Apply the border Styles.Comment Date
block styles take precedence over global Styles.Comment Date
block borders display correctly in both the Editor and Frontend.Screenshots or Screencast
add-border-support-in-comment-date.mp4