Open
Conversation
akasunil
added
[Type] Enhancement
|
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. |
Contributor
aaronrobertshaw
left a comment
aaronrobertshaw
left a comment
There was a problem hiding this comment.
I appreciate all the effort into publishing PRs adopting more block supports @akasunil 💪
This one works well until we include a caption for the video. At that point the border wraps around the outer figure element which has the caption as a child.
This is inconsistent with the image block which will target the inner img element for the border rather than the wrapping figure. I believe we'll need to do the same here for the Video block.
To that end, the Image block.json file should help illustrate how that can be done. The Image block is a dynamic block though so a static block like the Form Input block will help show how to apply the border support classes and styles within the Video block's save function.
Essentially, the Video block here needs to:
- skip serializing the border block support classes and styles on the block's wrapper by using
"__experimentalSkipSerialization": truein its supports config - define the
selectors.borderCSS selector needed for global styles to target the inner video element - manually apply the border support classes and styles in both the edit component and save function
It shouldn't be as painful as that sounds but let me know how you go. I'm happy to answer any questions you have, review as often as required, or help give the code a nudge.
Lastly, for anyone else coming to review this PR, the adoption of border radius is consistent with other blocks given the current padding support. The general consensus is that we should lean into user freedom rather than restrict them here due to the possibility that the inner video won't be clipped when the border radius is too high.
Thanks again for the work on this one 🙇
…add/video-border-support
|
Size Change: +217 B (+0.01%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 10305af. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10557619491
|
Member
Author
|
It should work fine now. video-block-border-styles.webm |
Contributor
aaronrobertshaw
left a comment
aaronrobertshaw
left a comment
There was a problem hiding this comment.
Thanks for the continued iterations on this one @akasunil 👍
With the border styles now applied directly to the video I think this is working more as expected. There is one catch though comparing the Video and Image blocks.
With the border radius applied to an image, there's no real downside. For the Video block though, it risks clipping the playback controls. With the block's padding being applied to the Video block's wrapper and not the video element there's not much control to mitigate this.
I'd like to get some advice from our design gurus on whether it is ok to proceed with the current approach. A compromise could be to not support border radius.
Personally, I'm leaning towards keeping the border radius support and allowing users to use their discretion on how much is too much when it comes to border radius.
akasunil
commented
Aug 26, 2024
I agree, especially because it's possible to hide the Playback controls already now anyway. |
Member
Author
|
Thank you @aaronrobertshaw and @hanneslsm If there are no more feedbacks, can we ship this PR? I have re-based it with latest trunk. |
Contributor
With the 6.7 release in its final stages, we should hold off until after the release so our design gurus can weigh in and confirm the general thinking. As this is an enhancement and not a critical bug fix we can avoid to take our time with it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
Add border block support to the
Videoblock.Part of #43247
Why?
Videoblock is missing border support.How?
Adds the border block support in block.json
Testing Instructions
Videoblock's border is configurable via Global StylesVideoblock and Apply the border stylesWatch attached video for more information.
Screenshots or screencast
Blog-Home-.-Template-.-gutenberg-.-Editor-.-WordPress-video-block.webm