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

Fix image caption margins #9566

Merged
merged 1 commit into from Sep 5, 2018

Conversation

Projects
None yet
3 participants
@jasmussen
Contributor

jasmussen commented Sep 3, 2018

In #7721, we introduced a new wrapping div for the image block.

As part of that, the margin for the figure was also zeroed out. This is what caused the regression.

Most themes don't touch the figure, and it's born with 1em margin top and bottom. In testing, however, we noticed that some themes do provide figure styles, and they zero them out.

As such, this PR fixes the regression by:

  • Removing the regression. Situations with figures that worked prior to the regression will work the same.
  • Adding comments to the code to prevent this from happening again.
  • Adding an additional bottom margin to the caption style, to ensure themes that zero out the figure have some bottom margin.

This PR adds a 1em bottom margin to all captions. This margin collapses if the figcaption is inside a figure, but is still there in case it's inside a div.

Before:

before

After:

screen shot 2018-09-03 at 13 13 28

screen shot 2018-09-03 at 13 13 23


To test, insert an image, a video, and an embed. Add captions. Try various alignments. Verify it looks good in all cases.

@jasmussen jasmussen self-assigned this Sep 3, 2018

@jasmussen jasmussen requested review from pento and mtias Sep 3, 2018

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 3, 2018

Contributor

Actually, turns out https://github.com/WordPress/gutenberg/pull/7721/files#diff-d1d037caeeea85a91038b28fa70cd435R119 was what caused the regression. Let me try a quick alternate fix.

Contributor

jasmussen commented Sep 3, 2018

Actually, turns out https://github.com/WordPress/gutenberg/pull/7721/files#diff-d1d037caeeea85a91038b28fa70cd435R119 was what caused the regression. Let me try a quick alternate fix.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 3, 2018

Contributor

Okay, so my initial assessment was incorrect, with regards to the figures margin collapsing due to being inside a div. The problematic part was simply zeroing out the margin on the figure. In 6fc47c4 I reverted that, and as a side effect we don't need the separate centering fix.

However why did I zero out the margin on the figure in the first place? Well, because the figure also has left and right margins, and when the image is wide or fullwide, that margin should probably not be there to complexify things.

Tagging @kjellr also for a sanity check.

Contributor

jasmussen commented Sep 3, 2018

Okay, so my initial assessment was incorrect, with regards to the figures margin collapsing due to being inside a div. The problematic part was simply zeroing out the margin on the figure. In 6fc47c4 I reverted that, and as a side effect we don't need the separate centering fix.

However why did I zero out the margin on the figure in the first place? Well, because the figure also has left and right margins, and when the image is wide or fullwide, that margin should probably not be there to complexify things.

Tagging @kjellr also for a sanity check.

@jasmussen jasmussen requested a review from kjellr Sep 3, 2018

Fix image caption margins
In #7721, we introduced a new wrapping div for the image block.

As part of that, the margin for the figure was also zeroed out. This is what caused the regression.

Most themes don't touch the figure, and it's born with 1em margin top and bottom. In testing, however, we noticed that some themes _do_ provide figure styles, and they zero them out.

As such, this PR fixes the regression by:

- Removing the regression. Situations with figures that worked prior to the regression will work the same.
- Adding comments to the code to prevent this from happening again.
- Adding an additional bottom margin to the caption style, to ensure themes that zero out the figure have some bottom margin.

This PR adds a 1em bottom margin to all captions. This margin collapses if the figcaption is inside a figure, but is still there in case it's inside a div.
@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 3, 2018

Contributor

Okay, this is ready for a final review.

Contributor

jasmussen commented Sep 3, 2018

Okay, this is ready for a final review.

@@ -295,6 +295,7 @@
@mixin caption-style() {
margin-top: 0.5em;
margin-bottom: 1em;

This comment has been minimized.

@jasmussen

jasmussen Sep 3, 2018

Contributor

This is necessary for themes that zero out the margin on the figure, which many themes do.

Could be worth visiting separately, adding a default margin to the image block, so they don't all stack up next to each other. Or not? Maybe that's squarely theme territory. Could be a theme.scss property?

@jasmussen

jasmussen Sep 3, 2018

Contributor

This is necessary for themes that zero out the margin on the figure, which many themes do.

Could be worth visiting separately, adding a default margin to the image block, so they don't all stack up next to each other. Or not? Maybe that's squarely theme territory. Could be a theme.scss property?

This comment has been minimized.

@kjellr

kjellr Sep 3, 2018

Contributor

This is necessary for themes that zero out the margin on the figure, which many themes do.

Good call, this makes sure things don't look broken out of the box on many themes (TwentyFifteen, Independent Publisher 2, etc.)

Could be worth visiting separately, adding a default margin to the image block, so they don't all stack up next to each other. Or not? Maybe that's squarely theme territory. Could be a theme.scss property?

This is definitely worth exploring. Ideally, images inserted via Gutenberg (inside of figure/div wrappers) are styled exactly like images inserted normally (without those wrappers). Not sure the right answer, but it's worth an exploration.

@kjellr

kjellr Sep 3, 2018

Contributor

This is necessary for themes that zero out the margin on the figure, which many themes do.

Good call, this makes sure things don't look broken out of the box on many themes (TwentyFifteen, Independent Publisher 2, etc.)

Could be worth visiting separately, adding a default margin to the image block, so they don't all stack up next to each other. Or not? Maybe that's squarely theme territory. Could be a theme.scss property?

This is definitely worth exploring. Ideally, images inserted via Gutenberg (inside of figure/div wrappers) are styled exactly like images inserted normally (without those wrappers). Not sure the right answer, but it's worth an exploration.

@@ -20,7 +20,11 @@
.aligncenter,
&.is-resized {
display: table;
margin: 0;
// The figure is born with left and right margin.

This comment has been minimized.

@jasmussen

jasmussen Sep 3, 2018

Contributor

Note the removal of top and bottom margin rules. We let the theme handle those.

The figure is born with left and right margins, though, and unless we zero them out left/right/fullwide images will look weird.

@jasmussen

jasmussen Sep 3, 2018

Contributor

Note the removal of top and bottom margin rules. We let the theme handle those.

The figure is born with left and right margins, though, and unless we zero them out left/right/fullwide images will look weird.

.aligncenter {
margin: 0 auto;
margin-left: auto;

This comment has been minimized.

@jasmussen

jasmussen Sep 3, 2018

Contributor

We still need to customize the centering, but rules here were rewritten to refer to the previous rules above.

@jasmussen

jasmussen Sep 3, 2018

Contributor

We still need to customize the centering, but rules here were rewritten to refer to the previous rules above.

@kjellr

This comment has been minimized.

Show comment
Hide comment
@kjellr

kjellr Sep 3, 2018

Contributor

I gave this a spin, and it works as anticipated. Here are a few screenshots:

Twenty Sixteen
twentysixteen

Twenty Seventeen
twentyseventeen

Twenty Fifteen
twentyfifteen

Gutenberg Starter Theme
screen shot 2018-09-03 at 3 02 16 pm

Independent Publisher 2
ip2

It's kind of unfortunate that so many of these themes show absolutely no vertical margins between image blocks w/no captions. But as noted above, I think we can explore a solution to that separately.

During testing, I noticed that video blocks don't seem to be inheriting captions styles, but I can reproduce that outside of this branch, so I'll file a separate bug. 👍

Contributor

kjellr commented Sep 3, 2018

I gave this a spin, and it works as anticipated. Here are a few screenshots:

Twenty Sixteen
twentysixteen

Twenty Seventeen
twentyseventeen

Twenty Fifteen
twentyfifteen

Gutenberg Starter Theme
screen shot 2018-09-03 at 3 02 16 pm

Independent Publisher 2
ip2

It's kind of unfortunate that so many of these themes show absolutely no vertical margins between image blocks w/no captions. But as noted above, I think we can explore a solution to that separately.

During testing, I noticed that video blocks don't seem to be inheriting captions styles, but I can reproduce that outside of this branch, so I'll file a separate bug. 👍

@jasmussen jasmussen added this to the 3.8 milestone Sep 4, 2018

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 4, 2018

Contributor

Thank you for thorough testing.

I created #9600 separately for the video captions issue.

Contributor

jasmussen commented Sep 4, 2018

Thank you for thorough testing.

I created #9600 separately for the video captions issue.

@youknowriad

LGTM 👍

@jasmussen jasmussen merged commit 5f9107e into master Sep 5, 2018

2 checks passed

codecov/project 50.37% remains the same compared to c9668e9
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jasmussen jasmussen deleted the try/fix-caption-spacing branch Sep 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment