In order to make media items easier to theme inside wysiwyg containers additional classes should be on the element.

This could be done in media_token_to_markup(). There is a note in this function about the concern of media elements not being wrapped in divs under some conditions. Perhaps we should supply some default CSS that displays items inline.

This patch adds a unique ID and media classes.

CommentFileSizeAuthor
#9 media-add_classes_to_classless_div-1872420-9.patch1.07 KBParisLiakos
#5 media-add_classes_to_classless_div-1872420-5.patch1.03 KBParisLiakos
#3 media-add_classes_to_classless_div-1872420-3.patch1020 bytesParisLiakos
media.diff268 bytesarthurf

Comments

ParisLiakos’s picture

ParisLiakos commented

not sure about id, since same file can be embedded more than once in the same page..but i agree on classes

arthurf’s picture

arthurf commented

Point taken about the id. It may also be worth adding some supporting classes for the view mode and the image style:

$attributes = array();
$attributes['class'] = array(
  $element['content']['file']['#style_name'],
  $element['content']['file']['#view_mode'],
   'media',
   'media-element',
 );
 $element['content']['#prefix'] = '<div ' . drupal_attributes($attributes) . ' >';
ParisLiakos’s picture

ParisLiakos commented
StatusFileSize
new media-add_classes_to_classless_div-1872420-3.patch1020 bytes

hmm i wanted to avoid div there when no additional fields are there so p tags wont break...eg if we have
<p>lorem ipsum <img /> lorem</p>
surrounding img in div would make it impossible to make it display inline...updated comment to explain this better

ParisLiakos’s picture

ParisLiakos commented
Status: Active » Needs review
ParisLiakos’s picture

ParisLiakos commented
StatusFileSize
new media-add_classes_to_classless_div-1872420-5.patch1.03 KB

hmm, should actually reset $attributes, since it is being used, before in the same scope

arthurf’s picture

arthurf commented

I'm wondering if we need another class such as media-element that designates this particular thing as something that media has inserted on the page. I like that you added media-STYLE. I'm just concerned that media by itself maybe overloaded- #content .media {...} might be too broad.

Personally I'm concerned about using different markup for elements that have fields and those that don't- I think it breaks some assumptions that we have about page markup and make theming more challenging. If the need is for inserted elements to be displayed inline let's add some css to handle that. But I'll save that argument for a new issue :)

ParisLiakos’s picture

ParisLiakos commented

Personally I'm concerned about using different markup for elements that have fields and those that don't- I think it breaks some assumptions that we have about page markup and make theming more challenging. If the need is for inserted elements to be displayed inline let's add some css to handle that. But I'll save that argument for a new issue :)

I couldnt agree more:) we need to find a way to make markup output consistent..also see #1807832: Add wrapper class input to media filter

I'm wondering if we need another class such as media-element that designates this particular thing as something that media has inserted on the page.

media-element class is currently used as class on files..i see now <img class="media-element" />
i think it does not make sense having the same class in the wrapper div and the file itself. maybe introduce a new class, media-wrapper or move media-element class from file to wrapper div

arthurf’s picture

arthurf commented

Personally I don't think the img should have a class of media-element- it is only one part of the element that is being put on the page- the fields are part of that element as well. I would like to see this only on the container div and not on the element itself- however this has the possibility to break people's theming if they've already started using it. I'm not sure what the policy on markup changes and releases is but if we do this we'd need to document it.

In the meantime, can we use media-element-container on the container div and not change the rest of the markup in this issue? Then we can maybe tackle making all the generated markup the same!

ParisLiakos’s picture

ParisLiakos commented
StatusFileSize
new media-add_classes_to_classless_div-1872420-9.patch1.07 KB

Agreed as well..img should not have any class..but i am pretty sure people use it already..lets discuss this as well in another issue, where we ll decide the overall markup of embedded media elements..
Patch attached adding media-element-container class..did not tested but i guess it works:)

arthurf’s picture

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

I tested this under these conditions:
* node published
* node unpublished
* node with multiple images in the body field
* node with multiple images with some formatters that display fields

Each test returns the expected value. Inserting a PDF did not work as expected, however since these are displayed as links by default this maybe the expected behavior, I'm not sure. Since this patch does not alter that behavior it is probably ready for commit though someone should probably verify that this is the desired behavior.

ParisLiakos’s picture

ParisLiakos commented
Status: Reviewed & tested by the community » Fixed

http://drupalcode.org/project/media.git/commit/133fe60

Follow up here: #1885432: Figure out a way to keep markup output consistent in media embeds

bbinkovitz’s picture

bbinkovitz commented

Maybe we should call those media elements that contain sub-elements "media compounds"? :-P

Status: Fixed » Closed (fixed)

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