Video: Add aspectRatio block support and utilties #67047
Conversation
This file contains 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
t-hamano
added
[Type] Enhancement
t-hamano
requested review from
tellthemachines,
ellatrix,
ajitbohra,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
|
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. |
t-hamano
commented
Nov 16, 2024
| @@ -12,6 +12,7 @@ export { | |||
| getSpacingClassesAndStyles as __experimentalGetSpacingClassesAndStyles, | |||
| getGapCSSValue as __experimentalGetGapCSSValue, | |||
| getShadowClassesAndStyles as __experimentalGetShadowClassesAndStyles, | |||
| getDimensionsClassesAndStyles as __experimentalGetDimensionsClassesAndStyles, | |||
There was a problem hiding this comment.
I'm wondering whether we should use the __experimental prefix.
There was a problem hiding this comment.
Good question.
The only reason I see to use experimental here is to advertise to third parties that the public method is subject to change, and should not be relied upon long term.
Is that the case here?
The counter argument is that anything can and will change: should we add __experimental* to everything? 😄
For my part, I don't see an obvious case for the experimental flag — it's possible folks have followed the naming pattern of other exported hooks in this file without considering it.
If third parties shouldn't be using this API then it could also be exported in the private APIs.
There was a problem hiding this comment.
Agreed 👍
I believe the other functions (maybe shadow excluded) were created before the private APIs approach solidified.
As part of other separate block support stabilization efforts, these utils are also on my radar to stabilize.
t-hamano
commented
Nov 16, 2024
Comment on lines
+44
to
+45
| className={ dimensionsProps.className } | ||
| style={ dimensionsProps.style } |
There was a problem hiding this comment.
As long as users don't add aspectRatio to block instances, the markup should remain the same, so I believe the block deprecation is not necessary.
There was a problem hiding this comment.
I'm not sure I'm following this comment, sorry, @t-hamano.
Isn't this code change explicitly about applying a user-selected aspect ratio on a block instance?
|
Size Change: +157 B (+0.01%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for putting this together @t-hamano 👍
My understanding was that the current aspect ratio block support doesn't cover the contain use case the original PR's author was chasing. Do you have plains to address that?
If there's a valid use case for the additional option, I think we might need to extend the block support first before adopting it.
Is the Scale control necessary? To me, the video should always fill the container, i.e. object-fit:cover is expected.
I appreciate you were skeptical of the use case, as per this comment, I'd just like to make sure that we're not dismissing use cases too easily.
If the intent of this PR is to make a decision that object-fit: cover is the only option the aspect ratio block support should support, then the PR description needs to reflect that.
Normally, differing opinions and decision making would get done on an issue. Would it be beneficial to step back and create an issue linking both open PRs, then get a consensus there?
I've requested review from the original PR's author, @up1512001, to see if the current behaviour here suits their needs. If it does, then all the discussion is moot and we can just move forward with this.
|
Thanks for the review!
This would certainly be ideal, perhaps we can continue the discussion in #60911. I would like to summarise there what I have understood from the discussion so far. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Alternative to #66946
Closes #60911
What?
This PR adds
dimensions.aspectRatiosupport to the video block.Why?
In #66946, we are trying to add
aspectRatiovia theDimensionsToolcomponent, i.e. as a custom block attribute.However, the
DimensionsToolcomponent is intended to be used in aToolsPanel, so this requires refactoring of theInspectorControls.Furthermore, we need to ensure that it is possible to migrate to block support in the future. In that case, I think it would be better to explore whether it is possible to implement it as block support from the start.
How?
To ensure that styles are applied correctly from both the global styles and block instance, skip serialization and apply the styles to the
videoelement inside the block.Testing Instructions
aspectRatioto the Video block via theme.json:{ "version": 3, "settings": { "appearanceTools": true, "layout": { "contentSize": "840px", "wideSize": "1100px" } }, "styles": { "blocks": { "core/video": { "dimensions": { "aspectRatio": "1/1" } } } } }Screenshots or screencast
568af937925f14584b0b8ce65772b03c.mp4