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

Prevent video block from causing AMP validation errors in stories #2255

Merged
merged 9 commits into from
May 11, 2019

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented May 3, 2019

Fixes #2109.

@googlebot googlebot added the cla: yes Signed the Google CLA label May 3, 2019
@westonruter westonruter changed the title Default video block in stories to autoplay:true and controls:false Prevent video block from causing AMP validation errors in stories May 3, 2019
@westonruter
Copy link
Member Author

  • Automatically populate poster image from selected video attachment's featured image. See e3d92a5 and #2240 (comment).

@swissspidy I'm not sure how to pull this off. I'm looking at something like this:

diff --git a/assets/src/components/with-amp-story-settings.js b/assets/src/components/with-amp-story-settings.js
index 67a0240e..042c53a0 100644
--- a/assets/src/components/with-amp-story-settings.js
+++ b/assets/src/components/with-amp-story-settings.js
@@ -48,6 +48,7 @@ const applyWithSelect = withSelect( ( select, props ) => {
 	const animationOrderEntry = animatedBlocks.find( ( { id } ) => id === props.clientId );
 
 	return {
+		select,
 		parentBlock: getBlock( getBlockRootClientId( props.clientId ) ),
 		// Use parent's clientId instead of anchor attribute.
 		// The attribute will be updated via subscribers.
@@ -125,6 +126,7 @@ export default createHigherOrderComponent(
 	( BlockEdit ) => {
 		return enhance( ( props ) => {
 			const {
+				select,
 				clientId,
 				name,
 				attributes,
@@ -156,6 +158,7 @@ export default createHigherOrderComponent(
 			}
 
 			const isImageBlock = 'core/image' === name;
+			const isVideoBlock = 'core/video' === name;
 			const isTextBlock = 'amp/amp-story-text' === name;
 			const needsTextSettings = BLOCKS_WITH_TEXT_SETTINGS.includes( name );
 			const isMovableBlock = ALLOWED_MOVABLE_BLOCKS.includes( name );
@@ -179,6 +182,14 @@ export default createHigherOrderComponent(
 				setAttributes( { caption: '' } );
 			}
 
+			// If we have a video set from an attachment but there is no poster, use the featured image of the video if available.
+			if ( isVideoBlock && attributes.id && ! attributes.poster ) {
+				const media = select( 'core' ).getMedia( attributes.id );
+				if ( media && media.featuredmedia ) {
+					setAttributes( { poster: media.featuredmedia.src } );
+				}
+			}
+
 			const minTextHeight = 20;
 			const minTextWidth = 30;

But it seems that a media REST API response does not have the featuredmedia populated. Help?

@westonruter
Copy link
Member Author

Also, I'm at a loss for how to hide the “autoplay” and “plays inline” toggles in the inspector controls.

@swissspidy
Copy link
Collaborator

Also, I'm at a loss for how to hide the “autoplay” and “plays inline” toggles in the inspector controls.

Same. There doesn't seem to be any class name that could be used for targeting these elements. Two possible options:

a) We can try to find a way to add some class names somewhere to allow for this
b) We just roll our own copy of the video block, at least in the meantime.

Related upstream issues:

WordPress/gutenberg#6023
WordPress/gutenberg#15450

@swissspidy
Copy link
Collaborator

But it seems that a media REST API response does not have the featuredmedia populated. Help?

Indeed. Looks like that's because the attachment post type does not officially support thumbnails, yet the UI for it is exposed in the admin... 🤷‍♂

I tried to come up with a workaround and 2a430d4 seems to do the trick.

@westonruter westonruter marked this pull request as ready for review May 11, 2019 01:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants