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

Support converting YouTube iframes with 100% width to amp-youtube with fixed-height layout #6837

Merged
merged 1 commit into from
Jan 19, 2022

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Jan 13, 2022

Summary

Fixes #6834

The new AMP_YouTube_Embed_Handler::amend_fixed_height_layout() method here is not ideal as it is only a partial implementation of the more robust AMP_Base_Sanitizer::set_layout() method. It would be better if AMP_Base_Sanitizer::set_layout() could be extracted into a trait (along with the methods it depends on) so that it could be reused both by AMP_Base_Sanitizer and AMP_Base_Embed_Handler. But this would expand the scope and necessitate some larger refactoring.

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@westonruter westonruter added this to the v2.2.1 milestone Jan 13, 2022
@westonruter westonruter marked this pull request as ready for review January 13, 2022 20:03
@github-actions
Copy link
Contributor

Plugin builds for e71f851 are ready 🛎️!

Copy link
Collaborator

@delawski delawski left a comment

Choose a reason for hiding this comment

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

The code looks good to me. When testing locally, an iframe with a YouTube video with width="100%" displays correctly (having layout="fixed-height"):

Screenshot 2022-01-19 at 14 14 42

@westonruter westonruter merged commit de491cd into develop Jan 19, 2022
@westonruter westonruter deleted the fix/one-hundred-percent-width-youtube-videos branch January 19, 2022 17:05
westonruter added a commit that referenced this pull request Jan 19, 2022
westonruter added a commit that referenced this pull request Jan 22, 2022
…r-amp-settings-panel

* 'develop' of github.com:ampproject/amp-wp:
  Render value as fallback instead of entire parent source object
  Fix logic inversion for sources.length
  Ensure `sources` is non-empty array
  Revert "Ensure error `sources` is never `null`"
  Ensure error `sources` is never `null`
  Ensure `sources` is iterated over only if it is non-empty array
  Fix test to account for gutenberg_render_layout_support_flag() (#6850)
  Support converting YouTube iframes with 100% width to amp-youtube with fixed-height (#6837)
  Update Gutenberg package dependencies
  Update Gutenberg package dependencies
  Allow composer normalize plugin to run
  Normalize Composer file
  Update Composer to use AMP toolbox v0.10.0
@westonruter westonruter added Changelogged Whether the issue/PR has been added to release notes. Embeds labels Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes. Embeds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Iframe having width="100%" with youtube embed link are not converted to fixed-height layout
2 participants