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

Constrain media blocks to content area width in frontend. #8399

Merged

Conversation

sarahmonster
Copy link
Member

As it stands right now, images and videos can potentially overflow the content area if their theme doesn't explicitly tell them not to. This doesn't affect the majority (guess) of newer themes, but is likely an issue with lots of older/non-responsive themes, including twentyten.

Giving a max-width of 100% to the image and video blocks ensures they won't break out of the content area of the site, regardless of whether the theme has styling that prevents overflow.

I tested this in a bunch of themes, both old and new, and it doesn't seem to introduce any egregious conflicts, but I may have missed something. I'd appreciate some extra 👀 on it!

Fixes #8334.

Before:

image

After:

image

@kjellr
Copy link
Contributor

kjellr commented Aug 2, 2018

I tested with a handful of themes, and haven't found any issues so far:


Twenty Ten:

localhost_8888__p 1 3


Twenty Seventeen:

localhost_8888__p 1

localhost_8888__p 1 8


Gutenberg Starter Theme:

localhost_8888__p 1 1

localhost_8888__p 1 6

(^ This was broken, but that's due to a theme bug. 🙂)


Twenty Sixteen

localhost_8888__p 1 2

localhost_8888__p 1 7


P2

localhost_8888__p 1 5


Independent Publisher

localhost_8888__p 1 4

localhost_8888__p 1 9


I can pick this up and test some more tomorrow. 👍

@chrisvanpatten
Copy link
Contributor

I vaguely remember having issues when using resized images with max-width 100%. Might be worth checking specifically.

@jasmussen
Copy link
Contributor

@chrisvanpatten does make a point, that for a resized image, the max-width doesn't apply (see https://github.com/WordPress/gutenberg/blob/master/core-blocks/image/style.scss#L11). But this is perhaps worth revisiting separately, as that's a tricky challenge on its own.

The big challenge is we have the figure to deal with, which is a block level element by default. If you add a caption to an image, how wide should the caption be? Ideally no wider than the image itself, which means if you resize that image to be small, say 200px, then the caption should also be no wider than the 200px.

Actually, let me try and see if I can address resized non floated images with captions in my floats branch — I think the table trick I'm using for floats might work here as well.

@@ -1,5 +1,10 @@
.wp-block-image {
width: fit-content;
max-width: 100%;
Copy link
Contributor

Choose a reason for hiding this comment

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

There are some spaces instead of tabs here.

max-width: 100%;

img {
max-width: 100%;
Copy link
Contributor

Choose a reason for hiding this comment

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

Also some spaces not tabs.

@jasmussen
Copy link
Contributor

I did some work in #7721 (comment) and now the resized images & captions should work.

Depending on which branch goes in first, this might need a rebase. But you shouldn't need to worry about floats, captions or resized images, that should be handled now.

@sarahmonster
Copy link
Member Author

Thanks for the checks @jasmussen! I've fixed those tabs-not-spaces. Happy to rebase as/when needed.

Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

Changes look good. Need to figure out why the tests aren't passing. I'll also be sure to rebase my floats PR to include this.

@sarahmonster
Copy link
Member Author

sarahmonster commented Aug 3, 2018

Thanks! I think @tofumatt is working on something to fix the tests that are breaking everywhere, or my CSS is more catastrophic than I'd intended.

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.

I tried a number of other themes this morning, and this seemed to work fine! 👏

Giving a max-width of 100% to the image and video blocks ensures
they won't break out of the content area of the site, regardless of
whether the theme has styling that prevents overflow.

Fixes #8334.
@sarahmonster sarahmonster force-pushed the update/constrain-image-and-video-widths-in-themes branch from e3010e3 to 5fe0387 Compare August 3, 2018 18:03
@sarahmonster
Copy link
Member Author

Hurray! Tests are passing now, huge thanks to @notnownikki, so I'm going to go ahead and merge this whilst everything still looks green.

Thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants