-
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
Embed Block: Border support added #66386
base: trunk
Are you sure you want to change the base?
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @benazeerhassan1909. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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, @benazeer-ben! 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.
Thank you for putting this together @benazeer-ben 🙇
I get the feeling this one is going to have to wrangle several edge cases given the various types of content the block can embed e.g. youtube videos, tweets, spotify and audio links etc.
Testing this PR as is, I've run into a couple of issues:
- To support border radius we should probably also support padding on the block. This might need design feedback as discussed on Video: Add border support.
- The current approach in this PR applies the border to the block's wrapper. The embed block can also include a caption. Other blocks such as the image block, apply the border to the actual element that needs the border leaving the caption outside it. I think this would be the expected behaviour here as well.
- When adding border or padding support we need to ensure the block has
box-sizing: border-box
styles so that it will respect any width its parent might be trying to apply to it.
Box sizing issue:
Embed blocks with different content and captions:
Next Steps
To move this PR forward while waiting on some design input regarding border-radius adoption, I think we can:
- Skip serialization of borders on the block wrapper and apply it to the embed's inner wrapper so the caption falls outside it. See Video: Add border block support #63777 for some clues on this approach.
- Add
box-sizing: border-box
to the block. (there's also a separate PR aimed at generically adding this style to blocks but it might be slow to land given the potential to cause visual regressions).
Thanks again for all your work on adopting block supports, it's appreciated 👍
Thanks @aaronrobertshaw for the detailed feedback and details. Implemented skip serialization of borders on the block wrapper and applied it to the embed's inner wrapper. For now padding support also added, if we get some design feedback we can change it accordingly. Please let me know if we need more tweak on this. Screenshare.-.2024-11-15.9_02_39.PM.mp4 |
} | ||
}, | ||
"selectors": { | ||
"border": ".wp-block-embed__wrapper " |
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.
The convention with these selectors is to explicitly include the block's class. In this case, .wp-block-embed
.
I'm not 100% whether it is required but it does make it clearer the intended target for the styles. That block class is also tweaked when generating block style variation selectors for the block.
One last nit, there's an unnecessary space at the end of that selector. Better safe than sorry, we should clean that up in case theres some theme.json processing somewhere that wasn't expecting it.
@@ -43,12 +43,29 @@ | |||
"supports": { | |||
"align": true, | |||
"spacing": { | |||
"margin": true | |||
"margin": true, | |||
"padding": true |
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.
This applies the padding to the Embed block's wrapper.
The justification for adding the padding support was so that it could be used to counter border radius and prevent the content from sticking out beyond the border.
This might need to be applied to the inner embed wrapper as well.
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 iterating on this 👍
I've given it a test and there are still a few issues. Most notably:
- Padding is applied to the wrapper so doesn't help with the border radius problem
- The custom selector for borders should probably also have the block class in it to ensure compatibility with block style variations.
- PR description and test instructions no longer match what the PR does
On that last point, I find it handy each time I iterate on a PR to re-test it in full asking myself some questions like:
- Have I changed what the PR does?
- If it does something new, how do I test that?
- If the PR is related to a block, are there different ways to use the block? e.g. testing the embed block with not just a video but all the other embed types.
Answering those questions then means you already have everything you need to update the PR description in a way that ensures things don't get missed and it's easier for reviewers.
Here's what I see when applying both padding and borders to various embeds. I'd expect the padding within the border, not outside it otherwise it's basically just a second margin.
What?
Adding block support for Embed block.
Part of #43247
Why?
Embed block is missing border support
How?
Adds the border block support via block.json
Testing Instructions
Screenshots or screencast
Backend:
Frontend: