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

Make the default layout responsive for the <amp-ooyala-player> block #1585

Merged
merged 1 commit into from
Nov 4, 2018

Conversation

kienstra
Copy link
Contributor

@kienstra kienstra commented Nov 2, 2018

Thanks to @csossi for reporting this in #1581.

Steps To Reproduce

  1. Activate Twenty Seventeen
  2. Create a new post with the block editor
  3. Add an AMP Ooyala Player block
  4. Set these values for it:
    Video embed code (required): Vxc2k0MDE6Y_C7J5podo3UDxlFxGaZrQ
    Player ID (required): 6440813504804d76ba35c8c787a4b33c
    Provider code for the account (required): 5zb2wxOlZcNCe_HVT3a6cawW298X
    Leave the defaults for the rest:

ooyala-player

  1. Publish the post, and go to the front-end
  2. Expected: The <amp-ooyala-player> remains within its container
  3. Actual: it overflows its container

ooyala-overflows-container

  • As Claudio pointed out, the default layout of fixed can cause it to overflow the container.
  • The layout of responsive seems to keep the width to the container size in Twenty Fifteen, Sixteen, and Seventeen.
  • So this changes the default layout from fixed to responsive.
  • Still, if the content area is floated, like in a mobile-first grid, a fixed layout might work better. Responsive layouts mean the element only has the width of its parent. But there's a <select> in the block editor to change this to something like fixed if needed.

Fixes #1581

As Claudio pointed out,
the default layout of fixed can cause it
to overflow the container.
responsive seems to keep the width to the container size
in Twenty Fifteen, Sixteen, and Seventeen.
Still, if the content area is floated,
like in a mobile-first grid, a fixed layout might work better.
Responsive layouts mean the element only has the width of its parent.
But there's a <select> in the block editor to change this.
@kienstra kienstra changed the title Make the default layout reponsive for the <amp-ooyala-player> block Make the default layout responsive for the <amp-ooyala-player> block Nov 2, 2018
@kienstra
Copy link
Contributor Author

kienstra commented Nov 2, 2018

Request For Review

Hi @westonruter,
Hope you're having a great day.

Could you please review this?

@kienstra kienstra requested a review from westonruter November 2, 2018 22:46
@kienstra kienstra changed the base branch from develop to 1.0 November 2, 2018 23:52
@westonruter westonruter merged commit 5ebf1a9 into 1.0 Nov 4, 2018
@westonruter westonruter deleted the fix/1581-ooyala-player branch November 4, 2018 17:17
@westonruter westonruter added this to the v1.0 milestone Nov 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants