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

Potential fix for embed space: move styles to theme.scss #10133

Closed
wants to merge 1 commit into from

Conversation

jasmussen
Copy link
Contributor

This PR fixes #10109.

But it does so in a semi-nuclear way, by removing the responsiveness by default, and making it something a theme author has to opt into by opting into Gutenberg "opinionated" styles. This might be the solution we go with, hence this PR, but I would love for us to consider it a "Plan B" while we explore better solutions.

This PR fixes #10109.

But it does so in a semi-nuclear way, by removing the responsiveness by default, and making it something a theme author has to opt into by opting into Gutenberg "opinionated" styles. This might be the solution we go with, hence this PR, but I would love for us to consider it a "Plan B" while we explore better solutions.
@jasmussen jasmussen added the [Type] Bug An existing feature does not function as intended label Sep 24, 2018
@jasmussen jasmussen self-assigned this Sep 24, 2018
@notnownikki
Copy link
Member

This makes me sad because it works so nicely for themes that don't have their own support...

I've been thinking about a block level toggle for responsiveness, so that if you're using a wide theme and embedding narrow videos you could tell it not to make them wide and ultra tall. I think we should do that anyway, but I'm not sure if it would be an acceptable solution for this too.

@jasmussen
Copy link
Contributor Author

I've been thinking about a block level toggle for responsiveness, so that if you're using a wide theme and embedding narrow videos you could tell it not to make them wide and ultra tall. I think we should do that anyway, but I'm not sure if it would be an acceptable solution for this too.

If we were to make a toggle, the thing we'd want to toggle off is the percentage padding hack that we use to create CSS responsiveness. This is what was causing the issue in #10109, becuase that theme already used the same hack, tied to different markup, to achieve responsiveness.

I agree this would be a bummer, and I hope we can think of other solutions.

@chrisvanpatten
Copy link
Contributor

I don't love this either, but as a plan B it makes sense.

I think it's worth waiting a bit though. So far we only have one reported theme that conflicts. If there's evidence the problem is more widespread maybe we could revisit, but as of right now it feels a bit like we'd be throwing the baby out with the bathwater to accommodate one case.

@notnownikki
Copy link
Member

I think it's worth doing the block level opt out anyway, because if you're embedding 9:16 ratio videos on a wide theme, then you might want to opt those out.

And if code changes can't work around this, and it's a problem on multiple themes, then a new media option that opts out of gutenberg responsiveness site-wide might be best?

@jasmussen
Copy link
Contributor Author

I think it's worth doing the block level opt out anyway, because if you're embedding 9:16 ratio videos on a wide theme, then you might want to opt those out.

🤘

That's pretty baller to do, though.

So that sounds like a "Make video responsive [on/off]" toggle, that defaults to on? I think this would be a good feature.

@notnownikki
Copy link
Member

So that sounds like a "Make video responsive [on/off]" toggle, that defaults to on?

Exactly! I have some updates to the category list to do, them I'm going to start on that.

@notnownikki
Copy link
Member

#10161 adds a toggle to make individual embed blocks responsive or not.

@jasmussen
Copy link
Contributor Author

Closing this for now, in favor of #10477.

@jasmussen jasmussen closed this Oct 11, 2018
@youknowriad youknowriad deleted the try/embeds-fix branch May 27, 2020 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Giant space above all Youtube video in Posts
3 participants