The attached patch adds media-image-left and media-image-right (along with the existing media-image class to give themers more control over styling embedded images. These classes are added whenever the style float: left or float: right are added.

This was originally raised in this issue, but was moved to it's own issue to avoid confusion. Note that unlike the patch in #1043570: Floating images left and right, this patch does not add any default styling for these new classes. It just adds the classes.

CommentFileSizeAuthor
add-float-classes.patch1.11 KBmikeker
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rootwork’s picture

rootwork
he/him
Primary language English
Location 🇺🇸 US-Pacific 🌎 Chinook (Multnomah, Clackamas) & Cowlitz lands 🌹 Portland, OR
CreditAttribution: rootwork commented

+1 on this!

Will try to test it out shortly and report back.

mrfelton’s picture

mrfelton CreditAttribution: mrfelton commented

+1

dboulet’s picture

dboulet CreditAttribution: dboulet commented

Yes, this would be great.

dboulet’s picture

dboulet CreditAttribution: dboulet commented
Version: 7.x-1.x-dev » 7.x-2.x-dev

Patch works well for me. I’m going to move this to the 2.x branch since it seems to be the main focus for development at the moment.

mikeker’s picture

mikeker CreditAttribution: mikeker commented

@dboulet: just to clarify, you used the patch against the 2.x branch and it was working fine?

Anyone willing to mark this RTBC? I'd love to see this added so I don't have to patch Media with each install. :)

dboulet’s picture

dboulet CreditAttribution: dboulet commented

Yes, it works fine on 2.x as well. The only thing that I think we should add is default CSS so that the floating works out of the box.

mikeker’s picture

mikeker CreditAttribution: mikeker commented

I thought about that (see the original patch, I had added some basic spacing around the image), but decided it was cleaner to leave that out. The floating should work out of the box as Media adds style="float: left" (or right) to the img tag. Please comment here if you DO NOT see the floats added and I can amend the patch. I haven't had a project using Media 2.x yet...

dboulet’s picture

dboulet CreditAttribution: dboulet commented

I see, I forgot that in my case I have an input filter that removes all inline styles. I like the idea of adding the spacing around the image, I think its worth adding. The CSS in your original patch looks good to me.

aaron’s picture

aaron CreditAttribution: aaron commented
Status: Needs review » Reviewed & tested by the community

works as advertised. i wonder about the notation of -1 != $var though; don't the drupal standards flip that?

mikeker’s picture

mikeker CreditAttribution: mikeker commented

The CONST == $variable order is a left over from a very embarrassing checkin I made about 16 years ago at a large, not-to-be-named-but-located-in-Redmond-WA software company. It was a typical if ($var = CONST) supposed-to-be-a-conditional-but-ooops bug that ended up shipping.

I blamed beer, but my boss blamed me. Can you imagine? :)

From then on, I trained myself to write if (CONST == $var) as that gives you a compile error if you leave out one of the '=' by accident. Granted, it doesn't matter with a not-equals, mostly force of habit at this point.

Anyhow, the Drupal coding standards and Drupal JS coding standards are mum on the topic. (Unless I missed it... in which case I'll blame beer again).

I'm happy to flip the conditionals if you like. I just want to see the patch committed so I don't have to re-patch Media with each upgrade! :)

Thanks, aaron.

JacobSingh’s picture

JacobSingh CreditAttribution: JacobSingh commented
Status: Reviewed & tested by the community » Fixed

Fixed in 7.2.x There are probably other larger fixes we have to make to the WYSIWYG style handling code, but this is a start to something people really need.
Nice work!

rootwork’s picture

rootwork
he/him
Primary language English
Location 🇺🇸 US-Pacific 🌎 Chinook (Multnomah, Clackamas) & Cowlitz lands 🌹 Portland, OR
CreditAttribution: rootwork commented

Woohoo!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

FiNeX’s picture

FiNeX CreditAttribution: FiNeX commented
Status: Closed (fixed) » Active

Current 2.x-dev doesn't have this feature. Has it been moved to an external module?

arthurf’s picture

arthurf CreditAttribution: arthurf commented

Note that there are new classes in 2.x from this issue: http://drupal.org/node/1872420 it does not have left/right support, but there are more theming options now.

jeremiahtre.in’s picture

jeremiahtre.in CreditAttribution: jeremiahtre.in commented

Last seen in 7.x-2.0-unstable6. Definitely not in dev.

mikeker’s picture

mikeker CreditAttribution: mikeker commented

Looks like the fix for this issue got dumped as part of the fix for #1451316: Clean up wysiwyg-media.js. I haven't had a chance to read through that thread as it's a fairly thorough refactoring of wysiwyg-media.js. It might be worth posting a comment in that issue to see if this was an intentional removal or if it should've been moved elsewhere in the code.

Please cross-post if anyone does inquire. Unfortunately, I don't have time to look into this further at the moment...

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Status: Active » Closed (fixed)

lets move this over to #1885432: Figure out a way to keep markup output consistent in media embeds