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.

Sign up for GitHub

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

Jump to bottom

Video block: Add poster image #9335

Merged
merged 6 commits into from
Aug 30, 2018
Merged

Video block: Add poster image #9335

merged 6 commits into from
Aug 30, 2018

Conversation

Soean
Copy link
Member

@Soean Soean commented Aug 24, 2018

Description

This PR adds the ability to add a poster image to the video block.

Fixes #4837

Screenshots

No poster selected
bildschirmfoto 2018-08-24 um 22 32 41

Poster selected
bildschirmfoto 2018-08-24 um 22 32 29

Workflow
video-poster

@Soean Soean added [Feature] Blocks Overall functionality of blocks [Feature] Media Anything that impacts the experience of managing media labels Aug 24, 2018
@benoitchantre benoitchantre mentioned this pull request Aug 24, 2018
Closed
@gziolo gziolo added this to the 3.7 milestone Aug 25, 2018
@gziolo gziolo requested review from SergioEstevao and gziolo August 25, 2018 05:42
@Soean Soean added the Needs Design Feedback Needs general design feedback. label Aug 25, 2018
@gziolo gziolo requested a review from jasmussen August 28, 2018 07:24
@jasmussen
Copy link
Contributor

This is pretty cool.

Seeing as this is not part of this PR, separately we should consider adding contextual help to the switches. Their effect is boolean and instant so it's okay to use switches, but we should have some help text below.

gziolo
gziolo reviewed Aug 28, 2018
View reviewed changes
packages/block-library/src/video/edit.js Outdated
onSelectPoster( image ) {
const { setAttributes, clientId } = this.props;
setAttributes( { poster: image.url } );
document.getElementById( 'block-' + clientId ).getElementsByTagName( 'video' )[ 0 ].load();
Copy link
Member

Choose a reason for hiding this comment

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

I think we should use ref in this case. See the related docs: https://reactjs.org/docs/refs-and-the-dom.html

Usage example in the existing code:
https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/block/edit-panel/index.js#L15
https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/block/edit-panel/index.js#L66
https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/block/edit-panel/index.js#L35

Copy link
Member

Choose a reason for hiding this comment

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

By the way, this logic should probably be moved to componenDidUpdate lifecycle method as it should be triggered before this.props.attributes.poster isn't updated.

SergioEstevao
SergioEstevao reviewed Aug 28, 2018
View reviewed changes
Copy link
Contributor

@SergioEstevao SergioEstevao left a comment

Choose a reason for hiding this comment

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

I will leave the code review for the experts, but in terms of the output/input HTML it's what we are expecting in Aztec. So looking great!

@gziolo
Copy link
Member

gziolo commented Aug 28, 2018

@SergioEstevao, awesome to hear that. By the way, are there any other missing bits to make Gutenberg fully compatible in Aztec? This one was the only one I was aware of.

@Soean
Copy link
Member Author

Soean commented Aug 28, 2018

@gziolo Thanks for your review. This is a better much better solution, I updated this PR.

gziolo
gziolo approved these changes Aug 29, 2018
View reviewed changes
Copy link
Member

@gziolo gziolo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This works great, thanks for adding the fix for the ref.

Would it be possible to improve the way poster is displayed? This is how it looks for my test video + image:

screen shot 2018-08-29 at 09 58 43

@gziolo
Copy link
Member

gziolo commented Aug 29, 2018

I added fix for the poster issue I shared. Kudos to @jasmussen for the solution. It still needs to be tested in IE11. I tested with the latest version of Chrome, FF and Safari.

@youknowriad youknowriad merged commit 208c0fa into master Aug 30, 2018
@youknowriad youknowriad deleted the add/video-poster branch August 30, 2018 08:35
@gziolo gziolo removed the Needs Design Feedback Needs general design feedback. label Aug 30, 2018
@harryfear
Copy link

harryfear commented Jan 16, 2020 •
edited
Loading

Hi, folks,

The video settings only appear if there is a Media Library video at play, but not for YouTube videos:
https://ibb.co/zZBdQR7

Whereas pre-Gutenburg, the Poster Image setting was present even on YT videos:
https://www.wpoptimus.com/775/add-change-poster-image-videos-wordpress/

So the 'classic to Gutenburg' issue #4837 isn't fixed yet, as I can see.

@Soean
Copy link
Member Author

Soean commented Jan 16, 2020

@harryfear This was a Plugin (https://wordpress.org/plugins/youtube-chromeless/), it was not possible in a standard WordPress installation.

@harryfear
Copy link

I'm not 100% sure if that's right? A Chromeless YouTube player is possible now, and has been as of v. 3.9?

@strarsis
Copy link
Contributor

strarsis commented Feb 2, 2022

For a core/cover block I cannot find the Video settings panel at all and no option for setting a poster/thumbnail.
The video currently doesn't load with a preview, which causes a kind of visual content flash.

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

@SergioEstevao SergioEstevao SergioEstevao left review comments

@gziolo gziolo gziolo approved these changes

@jasmussen jasmussen Awaiting requested review from jasmussen

Assignees
No one assigned
Labels
[Feature] Blocks Overall functionality of blocks [Feature] Media Anything that impacts the experience of managing media
Projects
None yet
Milestone
3.7
Development

Successfully merging this pull request may close these issues.

Support of poster image for video block is missing
7 participants
@Soean @jasmussen @gziolo @harryfear @strarsis @SergioEstevao @youknowriad