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 any iframe embed responsive in the editor #5333

Merged

Conversation

notnownikki
Copy link
Member

@notnownikki notnownikki commented Mar 1, 2018

Description

We used to reply on oEmbed's type information to make
YouTube embeds responsive in the editor. This change
removes that dependency, and looks for content embedded
in an iframe with a fixed size, and converts it to a responsive
iframe that keeps the correct aspect ratio when resized.

How Has This Been Tested?

  1. Create a new post with embeds for all the test content listed in components/sandbox/README.md
  2. Resize the browser window to a narrow width
  3. The YouTube videos should resize and keep the correct aspect ratio.

Types of changes

Bug fix

@niranjan-uma-shankar
Copy link

niranjan-uma-shankar commented Mar 6, 2018

This is of interest to the PR #4710 too, which uses the Sandbox component. Also, responsiveness is expected to work for any oEmbed video provider, right, for eg. TED, funnyordie etc.?

@notnownikki
Copy link
Member Author

This is for providers who embed their content in an iframe with fixed width and height, which is what YouTube does. This same approach can be used with other elements too, so we can extend this for different fixed width elements of we need to

@notnownikki
Copy link
Member Author

@niranjan-uma-shankar I just tested this with funnyordie, and they embed their content in a fixed size iframe, so this automatically applies there too. The iframe is initially set to the width of the editor, and as it gets resized it keeps to the width of the editor, and keeps the correct aspect ratio.

@notnownikki
Copy link
Member Author

Part of #5530

@jasmussen
Copy link
Contributor

This seems to me to be an improvement. Everything could always do with a better code review, than I can give. But given the state of embeds, I think we should consider merging this in so we don't get stuck. 👍 👍

@notnownikki notnownikki mentioned this pull request Mar 15, 2018
@notnownikki notnownikki force-pushed the update/sandbox-convert-fixed-iframe-to-responsive branch from 95ae197 to 072000a Compare April 30, 2018 13:09
@notnownikki notnownikki merged commit 2729474 into master May 10, 2018
@mtias mtias deleted the update/sandbox-convert-fixed-iframe-to-responsive branch May 17, 2018 16:59
@mtias mtias added the Mobile Web Viewport sizes for mobile and tablet devices label May 17, 2018
@mtias mtias added this to the 2.9 milestone May 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile Web Viewport sizes for mobile and tablet devices
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants