-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
feature: added aspect ration & scale option to video block #66946
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 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. |
@jasmussen removed extra spacer from aspect ratio controls. |
@@ -56,3 +56,8 @@ | |||
padding: 0; | |||
} | |||
} | |||
|
|||
// remove top border from tools panel from video block inspector controls. |
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.
Is there a way we can actually make the aspect ratio control part of the settings group, rather than add this CSS to override?
@@ -271,6 +290,24 @@ function VideoEdit( { | |||
</div> | |||
</MediaUploadCheck> | |||
</PanelBody> | |||
<ToolsPanel> |
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.
I.e. instead of adding a toolspanel here, just add the DimensionsTool to the existing PanelBody right above.
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.
Initially, I attempted that; however, adding DimensionsTool
within PanelBody
doesn't display in the controls.🤔
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.
That's why I need to overrider top border with CSS.
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 DimensionsTool
is intended for use within the context of a ToolsPanel
. You can see the aspect ratio, scale, and width/height controls using the ToolsPanelItem
in their respective files.
The ToolsPanel
component manages the context and state for the panel determining when controls should be displayed or not. Attempting to use the DimensionsTool
outside of a ToolsPanel
means nothing is telling the DimensionsTool
to render its controls.
@WordPress/gutenberg-components I wonder if there's a different way to accomplish this. Adding the inline CSS just for this case would add CSS drift. |
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!
To move forward with this PR, I think we need to consider two important points:
- This feature should probably be implemented as a block support, similar to the Cover block. That would eliminate a lot of custom code. However, the
aspectRatio
block support doesn't include the Scale control. - Is the Scale control necessary? To me, the video should always fill the container, i.e.
object-fit:cover
is expected.
cc @aaronrobertshaw @ajlende for visibility
@t-hamano if we only add Screen.Recording.2024-11-13.at.17.57.12.movLet me know is this desired place for |
I agree that this is a hacky use of ToolsPanel and should be avoided. Something like @t-hamano's suggestion sounds better to me. |
Thanks for the ping 👍
💯 Agreed. Extending the block support to cover all the desired functionality sounds like the long-term best option. The down side is that will delay enhancing the Video block for a while. There's a compromise in the short term though. Recently, image resolution options were added to the Cover block and faced a similar issue. That being, the A quick refactor of the Cover block's settings panel to be a Both the Cover and Image blocks already do this for their settings panel, could we not adopt the same approach here? |
@aaronrobertshaw Thanks for the feedback!
Aspect ratio support is implemented as block support in the Cover block, and as a block attribute in the Image block. Which implementation are you referring to? Personally, I think it might be a good idea to first explore whether it's possible with block support 🤔 |
I was referring to the Settings panel being a I'm not against exploring the block support angle. History has shown, though, that developing block supports takes additional time. So if the block support needs to be extended to support the desired options here, do we want to block this feature until we can work through the potential kinks there? Can we not implement this interim solution in a way that can be smoothly switched over to the block support when it is ready? I'm not very familiar with the aspect ratio block support, so these are genuine questions as I haven't had the chance to dig into code here yet. |
From my perspective, aspect ratio is generic enough that it deserves to be a block support. Any block with height and width support (most of the blocks ultimately) can benefit from aspect ratio. While I'm not against adding this adhoc if we want to ship this quickly, we should pay attention to the detail and only added in a way that can be moved later to a block support. Also, I would say that it should be our goal to make adding style block supports as simple and consistent as possible. |
I tried the following branch to see if we could apply the https://github.com/WordPress/gutenberg/compare/try/video-dimensions-props?expand=1 Presumably the It seems that the current functionality is mostly possible to implement, but I had to add a utility method ( d4f05b992a5c6493f6e9049f000cfbd0.mp4 |
@aaronrobertshaw I have created PR to refactor the settings panel, could you please check. 🙇♂️ |
@up1512001 Before considering refactoring the panel, I think we should consider whether it can be implemented as a block support. I've submitted #67047. |
What?
Added
Aspect ratio
&Scale
controls to video block.Why?
Closes #66933
Closes #60911
How?
DimensionsTool
added aspect ratio & scale control to video block.Testing Instructions
Screenshots or screencast
Screen.Recording.2024-11-13.at.01.04.27.mov