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

Do not make WordPress embeds responsive #10985

Merged
merged 2 commits into from
Oct 24, 2018

Conversation

notnownikki
Copy link
Member

Description

WordPress embeds are delivered in a fixed aspect ratio iframe, but the scripts that run to embed posts also manage the dimensions, so applying our responsive classes make it have weird spacing in the editor.

This change stops us applying responsive classes to embeds from WordPress.

How has this been tested?

Embed https://wordpress.org/news/2018/10/the-month-in-wordpress-september-2018/

Before this patch, there's a big space at the bottom of the block. After it, the spacing is fine.

Types of changes

Bug fix

@notnownikki notnownikki requested review from jasmussen and a team October 24, 2018 08:09
@notnownikki notnownikki added this to the 4.2 milestone Oct 24, 2018
@jasmussen
Copy link
Contributor

Great PR, I'd approve it for fixing this issue. I'm not code strong enough to suggest this is the right or wrong approach, but it seems like a blacklist — is that the right approach? I guess it would have to be right? Should we be whitelisting embeds that should be responsive instead? Thinking also in terms of #10440

@notnownikki
Copy link
Member Author

This seems to be the only one in core that has the issue that I can find. I would say that we fix it this way for now so that the 4.2 release isn't weird for WP embeds, and look at a longer term solution in a follow up that we start as soon as we're done with this.

@notnownikki
Copy link
Member Author

Actually, let's make this opt-out. This is too WordPress specific, and we can make it opt-out pretty easily.

@notnownikki notnownikki added the [Status] In Progress Tracking issues with work in progress label Oct 24, 2018
@notnownikki notnownikki removed the [Status] In Progress Tracking issues with work in progress label Oct 24, 2018
@notnownikki
Copy link
Member Author

Ok, this is opt-out now. The default embed block, used for embeds we don't know the details of, is not responsive, and the WordPress block is not responsive. It's a single line of code to opt a block out, should we find others with trouble, but we've done an extensive embed audit recently and WordPress was the only one I saw with this problem.

Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

Code/approach seems fine but won't this change a lot of user's content going forward?

packages/block-library/src/embed/index.js Show resolved Hide resolved
Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

Code makes sense to me.

@notnownikki notnownikki merged commit 69d0988 into master Oct 24, 2018
@notnownikki notnownikki deleted the fix/wordpress-embed-disable-responsiveness branch October 24, 2018 13:08
antpb pushed a commit to antpb/gutenberg that referenced this pull request Oct 26, 2018
@mtias mtias added [Type] Bug An existing feature does not function as intended [Block] Embed Affects the Embed Block labels Oct 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Embed Affects the Embed Block [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants