WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#23147 closed enhancement (fixed)

Twenty Twelve: Proper padding to floating images with caption

Reported by: TomasM Owned by: lancewillett
Milestone: 3.6 Priority: normal
Severity: normal Version: 3.5
Component: Bundled Theme Keywords: has-patch
Focuses: Cc:

Description

Floating image with caption isn't padded well. We would have to change style.CSS lines

img.alignleft {
	margin: 12px 24px 12px 0;
	margin: 0.857142857rem 1.714285714rem 0.857142857rem 0;
}
img.alignright {
	margin: 12px 0 12px 24px;
	margin: 0.857142857rem 0 0.857142857rem 1.714285714rem;
}
img.aligncenter {
	margin-top: 12px;
	margin-top: 0.857142857rem;
	margin-bottom: 12px;
	margin-bottom: 0.857142857rem;
}

to:

img.alignleft,
div.alignleft {
	margin: 12px 24px 12px 0;
	margin: 0.857142857rem 1.714285714rem 0.857142857rem 0;
}
img.alignright,
div.alignright {
	margin: 12px 0 12px 24px;
	margin: 0.857142857rem 0 0.857142857rem 1.714285714rem;
}
img.aligncenter,
div.aligncenter {
	margin-top: 12px;
	margin-top: 0.857142857rem;
	margin-bottom: 12px;
	margin-bottom: 0.857142857rem;
}

Attachments (1)

23147.patch (739 bytes) - added by lancewillett 2 years ago.
Add better spacing around aligned captions

Download all attachments as: .zip

Change History (20)

comment:1 @TomasM3 years ago

Looks like we can skip the .aligncenter part and leave only left and right:

img.alignleft,
div.alignleft {
	margin: 12px 24px 12px 0;
	margin: 0.857142857rem 1.714285714rem 0.857142857rem 0;
}
img.alignright,
div.alignright {
	margin: 12px 0 12px 24px;
	margin: 0.857142857rem 0 0.857142857rem 1.714285714rem;
}

comment:2 @lancewillett2 years ago

  • Keywords needs-patch reporter-feedback added; dev-feedback removed
  • Milestone changed from Awaiting Review to 3.6

Hi TomasM,
Could you please add screenshots showing the visual problem, or a URL if you have a live site to see it on?

Also, browser/OS information is helpful in case it's specific to one browser or OS.

comment:3 @TomasM2 years ago

Win8 same on latest FF and chrome.

Before fix: http://imgur.com/eUMUCWm,eRVTNJd#1

After fix: http://imgur.com/eUMUCWm,eRVTNJd#0

comment:4 @lancewillett2 years ago

Would you be able to post links to a live example (so we can inspect the HTML and CSS). Not sure why you have div in the selector, the theme should already work well with image captions using wp-caption selector to add a bit of padding.

comment:5 @TomasM2 years ago

.wp-caption adds only general padding for the whole element, but as we can see from the original css

img.alignleft {
	margin: 12px 24px 12px 0;
	margin: 0.857142857rem 1.714285714rem 0.857142857rem 0;
}

Rules for the aligned elements apply only if that element is image, caption is not image, so that's why we should add div selector, otherwise we could remove img, selector I would say, so every aligned element would get those rules applied:

For a limited time you can see it here (with original, not fixed CSS):

http://test.mbccopnetwork.org/urjeet-a-patel-md

Last edited 2 years ago by TomasM (previous) (diff)

comment:6 @lancewillett2 years ago

I see the issue now, thank you. I don't agree with adding div selector, though. Instead, the padding for wp-caption should be improved. So if right-aligned, give a bit more left padding, if left-aligned, a bit more right padding.

Do you want to work up a patch for that? I'm happy to review it and help test.

comment:7 @TomasM2 years ago

Or I guess we could do like this:

.alignleft img {
	margin: 12px 24px 12px 0;
	margin: 0.857142857rem 1.714285714rem 0.857142857rem 0;
}

If we want to apply those rule only for images, but I would say it's better to leave it plain, so people could use .alignleft for any element.

comment:8 @TomasM2 years ago

Sorry, but I don't use any version control software at this moment, will have to leave it for you guys ;)

comment:9 @TomasM2 years ago

I would vote for the plain .alignleft and .alignright:

.alignleft {
	margin: 12px 24px 12px 0;
	margin: 0.857142857rem 1.714285714rem 0.857142857rem 0;
}

It would work for all elements and we would not need to change the wp-caption thing.

Last edited 2 years ago by TomasM (previous) (diff)

comment:10 @lancewillett2 years ago

I would vote for the plain .alignleft and .alignright:

Not a great idea, it'd break a lot of things. People already have workarounds or CSS fixes, so we can't do that broad of an approach.

Thanks for all the screenshot and demo site info, that's really helpful. I'll work on a patch.

comment:11 @lancewillett2 years ago

  • Keywords reporter-feedback removed

@lancewillett2 years ago

Add better spacing around aligned captions

comment:12 @DrewAPicture2 years ago

23147.patch works for me.

comment:13 @DrewAPicture2 years ago

  • Keywords has-patch added; needs-patch removed

comment:14 @lancewillett2 years ago

  • Owner set to lancewillett
  • Resolution set to fixed
  • Status changed from new to closed

In 23472:

Twenty Twelve: better layout for floated image captions. Fixes #23147.

comment:15 follow-up: @TomasM2 years ago

Sorry for bugging, but I guess you forgot to put a space between classes ;)

.wp-caption.alignleft

comment:16 in reply to: ↑ 15 @lancewillett2 years ago

Replying to TomasM:

Sorry for bugging, but I guess you forgot to put a space between classes ;)

.wp-caption.alignleft

It's not a mistake, no. Did you test it?

comment:17 @TomasM2 years ago

Yes, it works, but I thought it was WP convention to have spaces between classes.

Sorry, again!

comment:18 @lancewillett2 years ago

Oh, no problem, I'm happy to explain.

In this case, the CSS selector is two class values on the same element: .one.two so they need to be tied together without a space. If it was the case of two elements (one inside the other) it'd need the space.

It's not WP conventions, just plain old CSS selectors. :)

comment:18 @TomasM2 years ago

Now I see that: http://css-tricks.com/multiple-class-id-selectors/

I've never heard about this, but I guess it's better later than never :)

Note: See TracTickets for help on using tickets.