Skip to content
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

Video: Add aspectRatio block support and utilties #67047

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/reference-guides/core-blocks.md
Original file line number Diff line number Diff line change
Expand Up @@ -957,7 +957,7 @@ Embed a video from your media library or upload a new one. ([Source](https://git

- **Name:** core/video
- **Category:** media
- **Supports:** align, anchor, interactivity (clientNavigation), spacing (margin, padding)
- **Supports:** align, anchor, dimensions (aspectRatio), interactivity (clientNavigation), spacing (margin, padding)
- **Attributes:** autoplay, blob, caption, controls, id, loop, muted, playsInline, poster, preload, src, tracks

<!-- END TOKEN Autogenerated - DO NOT EDIT -->
1 change: 1 addition & 0 deletions packages/block-editor/src/hooks/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ export { getBorderClassesAndStyles, useBorderProps } from './use-border-props';
export { getShadowClassesAndStyles } from './use-shadow-props';
export { getColorClassesAndStyles, useColorProps } from './use-color-props';
export { getSpacingClassesAndStyles } from './use-spacing-props';
export { getDimensionsClassesAndStyles } from './use-dimensions-props';
export { getTypographyClassesAndStyles } from './use-typography-props';
export { getGapCSSValue } from './gap';
export { useCachedTruthy } from './use-cached-truthy';
Expand Down
31 changes: 31 additions & 0 deletions packages/block-editor/src/hooks/use-dimensions-props.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/**
* Internal dependencies
*/
import { getInlineStyles } from './style';

// This utility is intended to assist where the serialization of the Dimensions
// block support is being skipped for a block but the dimensions related CSS
// styles still need to be generated so they can be applied to inner elements.

/**
* Provides the CSS class names and inline styles for a block's dimensions support
* attributes.
*
* @param {Object} attributes Block attributes.
*
* @return {Object} Dimensions block support derived CSS classes & styles.
*/
export function getDimensionsClassesAndStyles( attributes ) {
const { style } = attributes;

// Collect inline styles for dimensions.
const dimensionsStyles = style?.dimensions || {};
const styleProp = getInlineStyles( { dimensions: dimensionsStyles } );

return {
className: dimensionsStyles.aspectRatio
? 'has-aspect-ratio'
: undefined,
style: styleProp,
};
}
1 change: 1 addition & 0 deletions packages/block-editor/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ export {
getSpacingClassesAndStyles as __experimentalGetSpacingClassesAndStyles,
getGapCSSValue as __experimentalGetGapCSSValue,
getShadowClassesAndStyles as __experimentalGetShadowClassesAndStyles,
getDimensionsClassesAndStyles as __experimentalGetDimensionsClassesAndStyles,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering whether we should use the __experimental prefix.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

useCachedTruthy,
useStyleOverride,
} from './hooks';
Expand Down
7 changes: 7 additions & 0 deletions packages/block-library/src/video/block.json
Original file line number Diff line number Diff line change
Expand Up @@ -93,10 +93,17 @@
"padding": false
}
},
"dimensions": {
"__experimentalSkipSerialization": true,
"aspectRatio": true
},
"interactivity": {
"clientNavigation": true
}
},
"selectors": {
"dimensions": ".wp-block-video video"
},
"editorStyle": "wp-block-video-editor",
"style": "wp-block-video"
}
5 changes: 5 additions & 0 deletions packages/block-library/src/video/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
MediaUploadCheck,
MediaReplaceFlow,
useBlockProps,
__experimentalGetDimensionsClassesAndStyles as useDimensionsProps,
} from '@wordpress/block-editor';
import { useRef, useEffect, useState } from '@wordpress/element';
import { __, sprintf } from '@wordpress/i18n';
Expand Down Expand Up @@ -59,6 +60,8 @@ function VideoEdit( {
const { id, controls, poster, src, tracks } = attributes;
const [ temporaryURL, setTemporaryURL ] = useState( attributes.blob );

const dimensionsProps = useDimensionsProps( attributes );

useUploadMediaFromBlobURL( {
url: temporaryURL,
allowedTypes: ALLOWED_MEDIA_TYPES,
Expand Down Expand Up @@ -284,6 +287,8 @@ function VideoEdit( {
poster={ poster }
src={ src || temporaryURL }
ref={ videoPlayer }
className={ dimensionsProps.className }
style={ dimensionsProps.style }
>
<Tracks tracks={ tracks } />
</video>
Expand Down
6 changes: 6 additions & 0 deletions packages/block-library/src/video/save.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
RichText,
useBlockProps,
__experimentalGetElementClassName,
__experimentalGetDimensionsClassesAndStyles as getDimensionsClassesAndStyles,
} from '@wordpress/block-editor';

/**
Expand All @@ -25,6 +26,9 @@ export default function save( { attributes } ) {
playsInline,
tracks,
} = attributes;

const dimensionsProps = getDimensionsClassesAndStyles( attributes );

return (
<figure { ...useBlockProps.save() }>
{ src && (
Expand All @@ -37,6 +41,8 @@ export default function save( { attributes } ) {
preload={ preload !== 'metadata' ? preload : undefined }
src={ src }
playsInline={ playsInline }
className={ dimensionsProps.className }
style={ dimensionsProps.style }
Comment on lines +44 to +45
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

>
<Tracks tracks={ tracks } />
</video>
Expand Down
1 change: 1 addition & 0 deletions packages/block-library/src/video/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
video {
width: 100%;
vertical-align: middle;
object-fit: cover;
}

@supports (position: sticky) {
Expand Down
Loading