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

Improve video poster image handling #2240

Merged
merged 8 commits into from
May 3, 2019

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented May 2, 2019

  • Ensure poster image is centered in editor as it will be on frontend.
  • Make sure video is displayed instead of poster image.
  • Add warning when no poster image is selected.
  • Show preview of poster similar to how is done for the featured image.

Fixes #2214.

No Poster Image Selected

image

Poster Image Selected

image

Video Plays in Editor

image

@googlebot googlebot added the cla: yes Signed the Google CLA label May 2, 2019
@westonruter westonruter marked this pull request as ready for review May 2, 2019 18:56
@westonruter westonruter requested a review from swissspidy May 2, 2019 18:56
@swissspidy
Copy link
Collaborator

I added a small change to automatically use the video's featured image as a poster if there is one. This reduces the likelihood of getting the warnings.

I also simplified the UI a little bit, there were too many elements to do the same action IMO.

Screenshots:

Screenshot 2019-05-03 at 16 51 36
Screenshot 2019-05-03 at 16 51 22
Screenshot 2019-05-03 at 16 51 10

@swissspidy swissspidy merged commit 4ed0cec into amp-stories-redux May 3, 2019
@swissspidy swissspidy deleted the fix/video-poster-image branch May 3, 2019 15:04
@westonruter
Copy link
Member Author

westonruter commented May 3, 2019

I added a small change to automatically use the video's featured image as a poster if there is one. This reduces the likelihood of getting the warnings.

Doesn't this run the risk of setting the poster to be totally unrelated to the video?

Oh, I misunderstood. The video's featured image:

image

I wasn't aware that this was normally a thing.

@westonruter
Copy link
Member Author

I also simplified the UI a little bit, there were too many elements to do the same action IMO.

Sure. But just to note that the elements I added were the same as for the featured image control.

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