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

Support 'preload' attribute for Video Block #7929

Merged
merged 6 commits into from Aug 21, 2018

Conversation

Projects
None yet
7 participants
@Soean
Member

Soean commented Jul 12, 2018

Description

This PR adds the preload attribute to the video block.
As in the Classic editor the default is "metadata" and no attribute is set.

If not set, its default value is browser-defined (i.e. each browser may have its own default value). The spec advises it to be set to metadata.
(https://developer.mozilla.org/en-US/docs/Web/HTML/Element/video#attr-preload).

auto sets preload="auto" and none sets preload="none"

The design is the same as the preload in Audio, see #7590

Gutenberg
bildschirmfoto 2018-07-12 um 13 20 03

Classic Editor
bildschirmfoto 2018-07-12 um 13 21 31

related #7501

@Soean

This comment has been minimized.

Show comment
Hide comment
@Soean

Soean Jul 19, 2018

Member

@jorgefilipecosta I think it's fine now, can you review it again?

Member

Soean commented Jul 19, 2018

@jorgefilipecosta I think it's fine now, can you review it again?

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Jul 19, 2018

Member

Thank you for the changes @Soean things tested well in my test 👍 I left a minor comment that we may consider addressing before the merge.

Member

jorgefilipecosta commented Jul 19, 2018

Thank you for the changes @Soean things tested well in my test 👍 I left a minor comment that we may consider addressing before the merge.

@Soean Soean added this to the 3.4 milestone Jul 20, 2018

@Soean

This comment has been minimized.

Show comment
Hide comment
@Soean

Soean Jul 26, 2018

Member

@jorgefilipecosta Thanks for your feedback. I don't think we should add it to edit, see #7929 (comment)

Member

Soean commented Jul 26, 2018

@jorgefilipecosta Thanks for your feedback. I don't think we should add it to edit, see #7929 (comment)

@pento pento modified the milestones: 3.4, 3.5 Jul 30, 2018

@gziolo gziolo modified the milestones: 3.5, 3.6 Aug 8, 2018

@gziolo gziolo requested a review from WordPress/gutenberg-core Aug 8, 2018

@tofumatt

This is all done slightly differently in the Audio block, does it make sense to replicate that approach?

Show outdated Hide outdated core-blocks/video/index.js
Show outdated Hide outdated core-blocks/video/index.js

@youknowriad youknowriad modified the milestones: 3.6, 3.7 Aug 15, 2018

@tofumatt tofumatt self-assigned this Aug 16, 2018

Soean and others added some commits Jul 12, 2018

@tofumatt

I think this is good and I pushed a little update that disables editor interaction with the tag and brings it in line with the audio component's implementation. I think this is good to go.

@tofumatt

Scratch that, I'm seeing some block validation fails from earlier blocks, looking into it now.

@tofumatt tofumatt merged commit 8755ebf into master Aug 21, 2018

2 checks passed

codecov/project Absolute coverage decreased by -0.05% but relative coverage increased by +15.74% compared to bc4d7a2
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tofumatt tofumatt deleted the add/preload-video branch Aug 21, 2018

@Copons

This comment has been minimized.

Show comment
Hide comment
@Copons

Copons Aug 22, 2018

Contributor

@tofumatt May I ask you more info regarding the decision of disabling the player controls?

While working on #8055 I got used to play the audio file to make sure it was uploaded correctly, so now it feels a bit weird that the player controls don't work anymore.
But more importantly though, imho the simple presence of controls gives to the users the expectation that they can interact with them.

As it is right now, there are no visual clues that those controls don't work (e.g. they are not "grayed out"), and so the player feels broken even if it isn't.

Contributor

Copons commented Aug 22, 2018

@tofumatt May I ask you more info regarding the decision of disabling the player controls?

While working on #8055 I got used to play the audio file to make sure it was uploaded correctly, so now it feels a bit weird that the player controls don't work anymore.
But more importantly though, imho the simple presence of controls gives to the users the expectation that they can interact with them.

As it is right now, there are no visual clues that those controls don't work (e.g. they are not "grayed out"), and so the player feels broken even if it isn't.

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Aug 22, 2018

Member

Right now clicking on the block in any way activates the controls, which is very jarring when the user just means to click on the block to edit it. I ran into this while testing the block in this branch, and we've used a similar pattern elsewhere for things like server-side rendered blocks with HTML comments that, when clicked, will activate a link: https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/latest-comments/edit.js

I can see the argument for testing the content, though if it was uploaded and working you'll see a preview frame and length the in the players. I think testing the content is better done in the preview screen, leaving the editing screen to edit/interact with the content in an editing context.

If we were to add controls I think they'd be better in the inspector or the toolbar anyway so they're:

  1. not triggered by the user clicking on the block
  2. able to be used when playback controls are disabled

If you wanted to see that implemented–feel free to file a bug and we could discuss it further. 😄

EDIT: Oh, and please cc me on the bug. 😅

Member

tofumatt commented Aug 22, 2018

Right now clicking on the block in any way activates the controls, which is very jarring when the user just means to click on the block to edit it. I ran into this while testing the block in this branch, and we've used a similar pattern elsewhere for things like server-side rendered blocks with HTML comments that, when clicked, will activate a link: https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/latest-comments/edit.js

I can see the argument for testing the content, though if it was uploaded and working you'll see a preview frame and length the in the players. I think testing the content is better done in the preview screen, leaving the editing screen to edit/interact with the content in an editing context.

If we were to add controls I think they'd be better in the inspector or the toolbar anyway so they're:

  1. not triggered by the user clicking on the block
  2. able to be used when playback controls are disabled

If you wanted to see that implemented–feel free to file a bug and we could discuss it further. 😄

EDIT: Oh, and please cc me on the bug. 😅

@Copons

This comment has been minimized.

Show comment
Hide comment
@Copons

Copons Aug 22, 2018

Contributor

@tofumatt Thanks for the answer!
I understand the reasoning now but I'm not entirely sure I'm sold on the solution 😛

I guess disabled controls are ok per se, but I'd like to know that they are disabled (for example with some disabled style) before clicking on them, panicking, and eventually maybe realize they are just for show. 🙂

Contributor

Copons commented Aug 22, 2018

@tofumatt Thanks for the answer!
I understand the reasoning now but I'm not entirely sure I'm sold on the solution 😛

I guess disabled controls are ok per se, but I'd like to know that they are disabled (for example with some disabled style) before clicking on them, panicking, and eventually maybe realize they are just for show. 🙂

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Aug 22, 2018

Member

I'd like to know that they are disabled

That's fair! The <Disabled> component we use to disable interaction with rendered editor elements doesn't really apply default styles to the audio/video controls. We could at least do this for those blocks; could you file an issue for that and cc me on it? I'll do my best to take a look! 😄

Member

tofumatt commented Aug 22, 2018

I'd like to know that they are disabled

That's fair! The <Disabled> component we use to disable interaction with rendered editor elements doesn't really apply default styles to the audio/video controls. We could at least do this for those blocks; could you file an issue for that and cc me on it? I'll do my best to take a look! 😄

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Aug 22, 2018

Member

(If you haven't made the issue yet, I can do it actually, sorry!)

Member

tofumatt commented Aug 22, 2018

(If you haven't made the issue yet, I can do it actually, sorry!)

@Copons

This comment has been minimized.

Show comment
Hide comment
@Copons

Copons Aug 22, 2018

Contributor

@tofumatt No worries! I'm off for the day but I'll open it first thing tomorrow.

Contributor

Copons commented Aug 22, 2018

@tofumatt No worries! I'm off for the day but I'll open it first thing tomorrow.

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Aug 22, 2018

Member

Too late! 😉 #9252

Member

tofumatt commented Aug 22, 2018

Too late! 😉 #9252

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment