WordPress.org

Make WordPress Core

Opened 18 months ago

Closed 17 months ago

Last modified 17 months 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 17 months ago.
Add better spacing around aligned captions

Download all attachments as: .zip

Change History (20)

comment:1 TomasM18 months 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 lancewillett17 months 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 TomasM17 months 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 lancewillett17 months 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 TomasM17 months 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 17 months ago by TomasM (previous) (diff)

comment:6 lancewillett17 months 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 TomasM17 months 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 TomasM17 months ago

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

comment:9 TomasM17 months 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 17 months ago by TomasM (previous) (diff)

comment:10 lancewillett17 months 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 lancewillett17 months ago

  • Keywords reporter-feedback removed

lancewillett17 months ago

Add better spacing around aligned captions

comment:12 DrewAPicture17 months ago

23147.patch works for me.

comment:13 DrewAPicture17 months ago

  • Keywords has-patch added; needs-patch removed

comment:14 lancewillett17 months 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: TomasM17 months ago

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

.wp-caption.alignleft

comment:16 in reply to: ↑ 15 lancewillett17 months 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 TomasM17 months ago

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

Sorry, again!

comment:18 lancewillett17 months 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 TomasM17 months 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.