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

Try to add a min-width to embeds. #13590

Merged
merged 3 commits into from
Feb 14, 2019
Merged

Try to add a min-width to embeds. #13590

merged 3 commits into from
Feb 14, 2019

Conversation

jasmussen
Copy link
Contributor

Embeds are increasingly responsive (as they should be). This branch is very much a "try" branch, in that it simply zeroes out the minimum width of embeds to make the Amazon book embed be responsive. This allows us to create a nice book recommendation grid like this.

screenshot 2019-01-30 at 12 52 10

However, Amazon themselves include a "Share" modal inside, which is 320px wide and is opacity: 0; (until invoked) instead of display: none;. So even though the embed itself is smart enough to not even show the Share button when there isn't room, that zero opacity share sheet still causes a horizontal scrollbar, which due to the sandboxing of iframes, we cannot fix.

So this PR is more of a discussion point: what can we do with these embeds? I'm not sure we can do that much. Notably Instagram widgets can't be smaller than 326px, so we can't do this in a blanket way either, or it'll just mess up other embeds.

What are your thoughts?

Embeds are increasingly responsive (as they should be). This branch is very much a "try" branch, in that it simply zeroes out the minimum width of embeds to make the Amazon book embed be responsive. This allows us to create a nice book recommendation grid like this.

However, Amazon themselves include a "Share" modal inside, which is 320px wide and is `opacity: 0;` (until invoked) instead of `display: none;`. So even though the embed itself is smart enough to not even show the Share button when there isn't room, that zero opacity share sheet still causes a horizontal scrollbar, which due to the sandboxing of iframes, we cannot fix.

So this PR is more of a discussion point: what can we do with these embeds? I'm not sure we can do that much. Notably Instagram widgets can't be smaller than 326px, so we can't do this in a blanket way either, or it'll just mess up other embeds.

What are your thoughts?
@youknowriad
Copy link
Contributor

One option could be to define a minimum width for each embed (or globally) ourselves, which means if there's not enough space, we'll have horizontal sidebars but outside the iframe.

@jasmussen
Copy link
Contributor Author

Yes, that would be an option, but like the oembeds, and the icons for each, it means we have to maintain the service for third parties that may change these things at any time. The Amazon share sheet bug might be fixed tomorrow. I could probably take a stab, but are we sure this is something WordPress should take responsibility for?

@gziolo gziolo added the [Block] Embed Affects the Embed Block label Jan 31, 2019
@gziolo gziolo added this to the 5.1 (Gutenberg) milestone Feb 5, 2019
Copy link
Contributor

@kjellr kjellr left a comment

Choose a reason for hiding this comment

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

Yes, that would be an option, but like the oembeds, and the icons for each, it means we have to maintain the service for third parties that may change these things at any time. The Amazon share sheet bug might be fixed tomorrow. I could probably take a stab, but are we sure this is something WordPress should take responsibility for?

I don't think it is. With the wide range of uncontrollable iframe behavior, we're not going to be able to get this right for everyone. 😕

I think this PR is pretty solid for now. I tested this out with some other embeds, and (while it still appears broken) it seems less broken this way. This also appears to fix problems with the placeholder state:

Before

screen shot 2019-02-07 at 2 46 38 pm

screen shot 2019-02-07 at 2 59 16 pm

After

screen shot 2019-02-07 at 2 54 24 pm

screen shot 2019-02-07 at 3 00 06 pm

@jasmussen
Copy link
Contributor Author

Thank you Kjell, appreciate those thoughts.

The placeholder state should ABSOLUTELY have the min-width applied. But I'm increasingly unsure what to do here. I remember us actually adding this min-width because a right-floated Instagram embed would be broken otherwise. So before we merge this, I think we do need some additional thinking around what we can do. Probably some min-width should be present, because people using this won't know it's the vendors fault and not ours, that their floated instagram image is cropped.

I'm waffling, in other words. Not sure how to approach this.

@kjellr
Copy link
Contributor

kjellr commented Feb 12, 2019

I remember us actually adding this min-width because a right-floated Instagram embed would be broken otherwise.

Blargh. Yeah, you're right about that. This PR breaks left/right alignment for those:

scroll

Maybe for now, we use this PR to apply the min-width to placeholders only, and punt the rest to a future world in which everybody makes their embeds responsive?

@jasmussen
Copy link
Contributor Author

I think that's a great micro improvement we can do, regardless of any subsequent embed challenges. Great thought Kjell, thanks.

Pushed a fix:

screenshot 2019-02-13 at 09 20 05

It uses :not — are we okay with that? Otherwise we have to add the min-width: 0; to the placeholder with a great deal of specificity, which I feel is worse.

Copy link
Contributor

@kjellr kjellr left a comment

Choose a reason for hiding this comment

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

Adding the specificity to the placeholder feels okay to me — especially if we're doing it alongside adding the min-width:

@include break-small() {
	min-width: 360px;

	&.components-placeholder {
		min-width: 0;
	}
}

Alternatively: all embeds are assigned a second class that's prefixed with wp-block-embed-* (wp-block-embed-intagram, wp-block-embed-youtube, etc.), so we could do something like this:

@include break-small() {
	&[class^="wp-block-embed-"] {
		min-width: 360px;
	}
}

This avoids :not, but replaces it with a wildcard, so I'm not sure that's better. 😄

I don't have a strong opinion on which of the three options is better, so I'll approve this as is. If you think one of those other options is stronger, feel free to swap it in.

This is a solid, small improvement. Thanks for getting it sorted out. 🙂

@jasmussen
Copy link
Contributor Author

I like your suggested fix. Let's go with that:

screenshot 2019-02-14 at 07 29 16

@jasmussen jasmussen merged commit af00541 into master Feb 14, 2019
@jasmussen jasmussen deleted the try/amazon-fixes branch February 14, 2019 06:59
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* Try to add a min-width to embeds.

Embeds are increasingly responsive (as they should be). This branch is very much a "try" branch, in that it simply zeroes out the minimum width of embeds to make the Amazon book embed be responsive. This allows us to create a nice book recommendation grid like this.

However, Amazon themselves include a "Share" modal inside, which is 320px wide and is `opacity: 0;` (until invoked) instead of `display: none;`. So even though the embed itself is smart enough to not even show the Share button when there isn't room, that zero opacity share sheet still causes a horizontal scrollbar, which due to the sandboxing of iframes, we cannot fix.

So this PR is more of a discussion point: what can we do with these embeds? I'm not sure we can do that much. Notably Instagram widgets can't be smaller than 326px, so we can't do this in a blanket way either, or it'll just mess up other embeds.

What are your thoughts?

* Make min-width not affect placeholders.

* Change to suggested fix.
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* Try to add a min-width to embeds.

Embeds are increasingly responsive (as they should be). This branch is very much a "try" branch, in that it simply zeroes out the minimum width of embeds to make the Amazon book embed be responsive. This allows us to create a nice book recommendation grid like this.

However, Amazon themselves include a "Share" modal inside, which is 320px wide and is `opacity: 0;` (until invoked) instead of `display: none;`. So even though the embed itself is smart enough to not even show the Share button when there isn't room, that zero opacity share sheet still causes a horizontal scrollbar, which due to the sandboxing of iframes, we cannot fix.

So this PR is more of a discussion point: what can we do with these embeds? I'm not sure we can do that much. Notably Instagram widgets can't be smaller than 326px, so we can't do this in a blanket way either, or it'll just mess up other embeds.

What are your thoughts?

* Make min-width not affect placeholders.

* Change to suggested fix.
This was referenced Apr 30, 2020
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants