Add video block attributes: Autoplay, Controls, Loop, Muted #7672

Soean · 2018-07-02T21:48:58Z

Description

This PR adds 4 additional attributes to the video block.

Autoplay
Controls
Loop
Muted

See: https://developer.mozilla.org/de/docs/Web/HTML/Element/video

Types of changes

Autoplay and Loop are just added in save not in edit.
Autoplay and Loop are already in the Classic Editor, Controls and Muted are new in WordPress.
Controls is default true

Related: #7501

Screenshots

danielbachhuber · 2018-07-02T22:49:19Z

core-blocks/video/edit.js

@@ -86,8 +101,37 @@ class VideoEdit extends Component {
 						/>
 					</Toolbar>
 				</BlockControls>
+				<InspectorControls>
+					<PanelBody title={ __( 'Video Settings' ) }>


Does it make sense to label this "Playback Controls", to follow #7322?

Generally, I think "Playback Controls" is more specific than, and thus preferable to, "Video Settings"

Thanks for your feedback. I changed it to Playback Controls. I think it makes sense for this block to have multiple panels, if we add alternate sources, poster, subtitles, ...

tofumatt

This looks good. I made a note about maybe making a HOC or something for that toggleAttribute pattern.

The other two notes I left I addressed as I think this is otherwise good to go and they were minor changes. If you're okay with them then

tofumatt · 2018-07-05T00:13:24Z

core-blocks/video/edit.js

+		this.toggleAttribute = this.toggleAttribute.bind( this );
+	}
+
+	toggleAttribute( attribute ) {


Seems like this would make a handy higher-order-component components could use as a sort of mixin, given it's the second place we've used this pattern (after Audio).

If you could either create a HOC for this and Audio (and probably other) components to use that'd be neat… or if that's too much of a pain could you at least file an issue with the "code quality" label for it and link to this comment? 😄

Thanks for your comment. I am not a React expert (yet). So I created an issue for this: #7717

tofumatt · 2018-07-05T00:36:22Z

core-blocks/video/edit.js

 				<figure className={ className }>
-					<video controls src={ src } />
+					<video
+						controls={ controls }


In edit mode, these attributes shouldn't be linked to the output. Check out the Audio edit for inspiration: https://github.com/WordPress/gutenberg/blob/master/core-blocks/audio/edit.js#L119

For instance: if you enable loop for the video and then click "play" in the editor the video will play constantly until you click it again. That's an annoying behaviour.

The InspectorControls tell you what settings are enabled, this is a good example of "what you see [should not be/is not] what you get" 😄

I think this should just stay as <video controls src={ src } />

We can remove Loop, Controls and Muted should be okay.

tofumatt

Just so you know I think the controls should be changed here, so I'll push a fix and if you agree, again: 🚢

If not please tell me why I'm wrong 😄

tofumatt · 2018-07-05T00:45:33Z

core-blocks/video/edit.js

@@ -86,6 +101,30 @@ class VideoEdit extends Component {
 						/>
 					</Toolbar>
 				</BlockControls>
+				<InspectorControls>
+					<PanelBody title={ __( 'Playback Controls' ) }>


I know this was already changed, but I actually think "Playback controls" for audio is maybe something I should have caught before and isn't the right fit.

Calling this "playback controls" when "autoplay" and "loop" aren't "controls" per-se is weird, and it also is redundant as you have "Playback Controls > Controls".

Realistically this should just be "Video Options" and the option underneath labelled "Controls" should arguably be "Playback Controls".

tofumatt · 2018-07-05T01:26:04Z

I understand autoplay and loop are enabled in the classic editor as you say, but there are usability issues with enabling them in the editing experience, and it's still easy to see them enabled in the sidebar, so I think it's worth changing that UX.

Soean · 2018-07-05T05:51:55Z

Thanks for your feedback. I created a HOC issue, I think controls and Muted preview should be fine in the editor.

gziolo · 2018-07-05T09:03:32Z

Have you tested it with old posts? save method produces different output, I bet it might cause all existing Video blocks to be recognized as externally modified by blocks validator.

tofumatt

Have you tested it with old posts? save method produces different output, I bet it might cause all existing Video blocks to be recognized as externally modified by blocks validator.

Shoot, you're right. This will need a deprecated save method, my bad. 😓

@Soean You'll need to accommodate for the markup change in how video blocks are saved now. You can read the doc here: https://github.com/WordPress/gutenberg/blob/master/docs/block-api/block-edit-save.md

gziolo · 2018-07-05T09:31:31Z

By the way, we finally need to write e2e test which ensures that Demo post renders properly - in general there are no dialogs informing that it has changed externally. Filed in #7719.

karmatosed

I am going to approve as right now using 4 switches is the right pattern. However I think we've reached the point of too many and maybe want to consider other approaches with this many options. That would be done in another issue.

Soean · 2018-07-11T21:19:58Z

@gziolo @tofumatt
There is no change for old posts with videos, because there was only controls before, which is now default and the three new attributes Loop, Autoplay and Muted are default toggled off.
I tested with an old video bock and there was no problem.

gziolo

I tested it with the Video block created in master and I can confirm that it works as expected. It looks like you can add new attributes as long as they aren't included in HTML markup by default.

StaggerLeee · 2018-07-20T19:28:37Z

You are doing it great. It will be something special.

Soean added 2 commits July 2, 2018 23:30

Add video block attributes

7e5bd6d

Regenrerate fixtures

8ed0045

Soean added [Feature] Blocks Overall functionality of blocks [Feature] Media Anything that impacts the experience of managing media labels Jul 2, 2018

danielbachhuber reviewed Jul 2, 2018

View reviewed changes

danielbachhuber added the Needs Design Feedback Needs general design feedback. label Jul 2, 2018

Soean added the [Type] Copy Issues or PRs that need copy editing assistance label Jul 2, 2018

Soean and others added 2 commits July 3, 2018 09:21

Change Panel title to Playback Controls

ba31ed5

fix: Maintain controls and consistent video tag in editor

03f4b07

tofumatt approved these changes Jul 5, 2018

View reviewed changes

tofumatt reviewed Jul 5, 2018

View reviewed changes

chore: Fix tests and wording

c39e5a6

Soean mentioned this pull request Jul 5, 2018

HOC: toggleAttribute #7717

Closed

tofumatt requested changes Jul 5, 2018

View reviewed changes

gziolo mentioned this pull request Jul 5, 2018

E2E tests: Add test case which will validate that blocks render properly when their definition changes #7719

Closed

karmatosed self-requested a review July 11, 2018 15:06

karmatosed approved these changes Jul 11, 2018

View reviewed changes

karmatosed removed the Needs Design Feedback Needs general design feedback. label Jul 11, 2018

gziolo approved these changes Jul 12, 2018

View reviewed changes

gziolo added this to the 3.3 milestone Jul 12, 2018

gziolo merged commit 72dcdcf into master Jul 12, 2018

gziolo deleted the update/video-block-attributes branch July 12, 2018 09:27

benoitchantre mentioned this pull request Jul 12, 2018

Video block: missing features compared to classic editor #7501

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add video block attributes: Autoplay, Controls, Loop, Muted #7672

Add video block attributes: Autoplay, Controls, Loop, Muted #7672

Soean commented Jul 2, 2018 •

edited

Loading

danielbachhuber Jul 2, 2018

Soean Jul 3, 2018

tofumatt left a comment

tofumatt Jul 5, 2018

Soean Jul 5, 2018

tofumatt Jul 5, 2018

Soean Jul 5, 2018 •

edited

Loading

tofumatt left a comment

tofumatt Jul 5, 2018

tofumatt commented Jul 5, 2018 •

edited

Loading

Soean commented Jul 5, 2018 •

edited

Loading

gziolo commented Jul 5, 2018

tofumatt left a comment

gziolo commented Jul 5, 2018 •

edited

Loading

karmatosed left a comment

Soean commented Jul 11, 2018

gziolo left a comment

StaggerLeee commented Jul 20, 2018

Add video block attributes: Autoplay, Controls, Loop, Muted #7672

Add video block attributes: Autoplay, Controls, Loop, Muted #7672

Conversation

Soean commented Jul 2, 2018 • edited Loading

Description

Types of changes

Screenshots

danielbachhuber Jul 2, 2018

Choose a reason for hiding this comment

Soean Jul 3, 2018

Choose a reason for hiding this comment

tofumatt left a comment

Choose a reason for hiding this comment

tofumatt Jul 5, 2018

Choose a reason for hiding this comment

Soean Jul 5, 2018

Choose a reason for hiding this comment

tofumatt Jul 5, 2018

Choose a reason for hiding this comment

Soean Jul 5, 2018 • edited Loading

Choose a reason for hiding this comment

tofumatt left a comment

Choose a reason for hiding this comment

tofumatt Jul 5, 2018

Choose a reason for hiding this comment

tofumatt commented Jul 5, 2018 • edited Loading

Soean commented Jul 5, 2018 • edited Loading

gziolo commented Jul 5, 2018

tofumatt left a comment

Choose a reason for hiding this comment

gziolo commented Jul 5, 2018 • edited Loading

karmatosed left a comment

Choose a reason for hiding this comment

Soean commented Jul 11, 2018

gziolo left a comment

Choose a reason for hiding this comment

StaggerLeee commented Jul 20, 2018

Soean commented Jul 2, 2018 •

edited

Loading

Soean Jul 5, 2018 •

edited

Loading

tofumatt commented Jul 5, 2018 •

edited

Loading

Soean commented Jul 5, 2018 •

edited

Loading

gziolo commented Jul 5, 2018 •

edited

Loading