Enable playing the Core Video block's video in the editor #2799

kienstra · 2019-07-12T01:39:54Z

Allows the video in the Core Video block to play in the editor
This forks the Core Video block's edit component
Ideally, this would just extend that component, and override the render() method.
But that caused a console warning, that suggested that the class extend Component instead of the Video BlockEdit
Also incorporates logic to automatically set the poster image upon video selection
Also prevents the poster warning to be shown in edit mode by inlining the withVideoPosterImageNotice logic within the component itself.
Prevents poster image to be removed as it is required. It can only be replaced.

Closes #2530.
Fixes #2808.

Ideally, this would just extend that component, and override the render() method. But that caused a console warning.

assets/css/amp-stories.css

kienstra · 2019-07-12T01:50:32Z

assets/src/stories-editor/components/video-block-edit-with-preview.js

+		// Set the block's src from the edit component's state, and switch off
+		// the editing UI.
+		if ( newSrc !== src ) {
+			// Omit the embed block logic, as that didn't seem to work.


Before, entering an embed URL below 'Insert from URL' was supposed to create an Embed block:

This is intentionally disabled for YouTube, but even other embeds like Vimeo don't seem to display.

Is it OK that I removed the logic to convert to embeds?

https://github.com/WordPress/gutenberg/blob/cb718ffdff57e35f05f4239ff03ceb476ae69a86/packages/block-library/src/video/edit.js#L111-L117

Entering a non-embed URL still works. But embed support will probably require copying a lot of other dependencies, like createUpgradedEmbedBlock(), making this file much longer.

Is it OK that I removed the logic to convert to embeds?

I suppose so. AMP Stories doesn't really support any external providers AFAIK.

cc @westonruter

AMP Story should support Twitter embed in theory.

Pasting a tweet URL in a video block and converting it to a twitter embed would be a bit unexpected, no?

True 😄 My comment was a bit out of context.

External video providers aren't supported yet, but support is planned soon: ampproject/amphtml#15847

assets/src/stories-editor/components/video-block-edit-with-preview.js

The Video block has a display that shows only the video, and one that allows editing. Before, I thought of letting the original block handle the display for editing. But it looks like that's not possible.

assets/src/stories-editor/index.js

This will probably be important to allow another PR to filter the mediaUpload to only allow certain types.

kienstra · 2019-07-12T03:13:32Z

Request For Review
Hi @swissspidy and @miina,
Could you please review this?

It allows the video in the Core Video block to play in the editor, where it was wrapped in <Disabled> before:

Though this might be overkill, as it forks most of the Core Vide block's edit component.

I couldn't figure out a way around this. I tried creating a new class that extends the Video edit component, like:

class VideoEditWithPreview extends InitialBlockEdit {
        render() {
               /* implements render */
        }
}

Ideally, this would simply override the render() method, and inherit the rest of the parent class's methods.

This caused a console warning that the component should instead extend Component.

Maybe there's another way around this that I haven't thought of.

Thanks, and have a great weekend!

assets/src/stories-editor/components/index.js

swissspidy · 2019-07-13T13:27:32Z

assets/src/stories-editor/components/video-block-edit-with-preview.js

+				<InspectorControls>
+					<PanelBody title={ __( 'Video Settings' ) }>
+						<ToggleControl
+							label={ __( 'Playback Controls' ) }


Uhm, it doesn't seem like controls are actually displayed in AMP Stories? The runtime seems to hide them no matter what.

@westonruter Can you confirm this? Should we just remove this option?

AFAIK this was already removed at some point.

Found also this: #2255 where controls was set to false by default.

Hmm so we should remove the controls setting, but keep the loop (default to true) and mute settings?

Even though mute doesn't seem to have any effect. When opening an AMP Story, sound is muted by default for the whole story (see upper right corner), and when I enable sound, the video is not muted.

Yep, the mute control doesn't seem to have any effect anyway, looks like "Loop" is the only control that actually does anything.

Seems like we could probably just add the loop and leave the rest out for now.

Added back the loop setting and removed the one for controls.

If loop is disabled, controls are still shown since one might want to re-start the video during editing.

swissspidy

1-2 open questions but otherwise works great.

swissspidy · 2019-07-15T13:07:21Z

As pointed out in #2808, I think we can now easily move the logic from withVideoPosterImageNotice into our custom block edit component. This would allow fixing #2808 and simplify the code there.

Video poster images are required. Clicking on the remove button doesn’t work anyway as the stories editor automatically assigns the poster image again.

kienstra · 2019-07-15T15:44:38Z

As pointed out in #2808, I think we can now easily move the logic from withVideoPosterImageNotice into our custom block edit component. This would allow fixing #2808 and simplify the code there.

Ah, good point.

swissspidy · 2019-07-15T18:34:45Z

Since I added some changes, I'd like another pair of eyes do the final review here 🙂

kienstra · 2019-07-15T19:37:22Z

@swissspidy, would it help if I reviewed the latest changes?

assets/src/stories-editor/components/custom-video-block-edit.js

swissspidy · 2019-07-15T19:45:57Z

@kienstra Sure, but you cannot approve your own PR :-)

Fork the Core Video block's edit component, to enable playing the video

c43016c

Ideally, this would just extend that component, and override the render() method. But that caused a console warning.

kienstra added the AMP Stories label Jul 12, 2019

googlebot added the cla: yes Signed the Google CLA label Jul 12, 2019

kienstra commented Jul 12, 2019

View reviewed changes

assets/css/amp-stories.css Show resolved Hide resolved

kienstra commented Jul 12, 2019

View reviewed changes

assets/src/stories-editor/components/video-block-edit-with-preview.js Outdated Show resolved Hide resolved

kienstra commented Jul 12, 2019

View reviewed changes

assets/src/stories-editor/components/video-block-edit-with-preview.js Outdated Show resolved Hide resolved

Add edit display component, which I had ommitted before

073eb6f

The Video block has a display that shows only the video, and one that allows editing. Before, I thought of letting the original block handle the display for editing. But it looks like that's not possible.

kienstra commented Jul 12, 2019

View reviewed changes

assets/src/stories-editor/index.js Outdated Show resolved Hide resolved

Add the compose call at the bottom, which I didn't add before

aa92a7d

This will probably be important to allow another PR to filter the mediaUpload to only allow certain types.

kienstra requested review from miina and swissspidy July 12, 2019 03:13

swissspidy reviewed Jul 13, 2019

View reviewed changes

assets/src/stories-editor/components/index.js Outdated Show resolved Hide resolved

swissspidy added 4 commits July 13, 2019 15:19

Merge branch 'develop' into add/video-previews-editor

76e1e06

Rename components

778f2e3

Add text domain

2a172ec

Amend docblock

5031279

swissspidy reviewed Jul 13, 2019

View reviewed changes

swissspidy requested a review from westonruter July 13, 2019 13:28

swissspidy added this to the v1.2.1 milestone Jul 13, 2019

swissspidy approved these changes Jul 13, 2019

View reviewed changes

swissspidy added 2 commits July 15, 2019 13:30

Merge branch 'develop' into add/video-previews-editor

19a886d

Add loop setting, remove controls setting

54bb1d2

swissspidy mentioned this pull request Jul 15, 2019

Video poster image warning shown in placeholder #2808

Closed

Do not allow removing the video poster

126fbff

Video poster images are required. Clicking on the remove button doesn’t work anyway as the stories editor automatically assigns the poster image again.

swissspidy self-requested a review July 15, 2019 13:22

Add video poster logic to custom block edit component

84a3e7e

swissspidy approved these changes Jul 15, 2019

View reviewed changes

miina reviewed Jul 15, 2019

View reviewed changes

assets/src/stories-editor/components/custom-video-block-edit.js Show resolved Hide resolved

miina approved these changes Jul 15, 2019

View reviewed changes

swissspidy merged commit e63d2c3 into develop Jul 15, 2019

swissspidy deleted the add/video-previews-editor branch July 15, 2019 20:19

This was referenced Jul 16, 2019

Enable video previews in editor #2530

Closed

Add video block size validation in the Media Library #2755

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable playing the Core Video block's video in the editor #2799

Enable playing the Core Video block's video in the editor #2799

kienstra commented Jul 12, 2019 •

edited by swissspidy

Loading

kienstra Jul 12, 2019 •

edited

Loading

swissspidy Jul 13, 2019

miina Jul 15, 2019

swissspidy Jul 15, 2019

miina Jul 15, 2019

westonruter Jul 15, 2019

kienstra commented Jul 12, 2019 •

edited

Loading

swissspidy Jul 13, 2019

miina Jul 15, 2019

swissspidy Jul 15, 2019

miina Jul 15, 2019

swissspidy Jul 15, 2019

swissspidy left a comment

swissspidy commented Jul 15, 2019

kienstra commented Jul 15, 2019

swissspidy commented Jul 15, 2019

kienstra commented Jul 15, 2019

swissspidy commented Jul 15, 2019

Enable playing the Core Video block's video in the editor #2799

Enable playing the Core Video block's video in the editor #2799

Conversation

kienstra commented Jul 12, 2019 • edited by swissspidy Loading

kienstra Jul 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kienstra commented Jul 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swissspidy left a comment

Choose a reason for hiding this comment

swissspidy commented Jul 15, 2019

kienstra commented Jul 15, 2019

swissspidy commented Jul 15, 2019

kienstra commented Jul 15, 2019

swissspidy commented Jul 15, 2019

kienstra commented Jul 12, 2019 •

edited by swissspidy

Loading

kienstra Jul 12, 2019 •

edited

Loading

kienstra commented Jul 12, 2019 •

edited

Loading