Problem/Motivation

Given the intended use for Paragraphs, this module should default to a hands-off approach when it comes to influencing the markup and styling of paragraph items. Currently the theme_paragraphs_view() function inserts wrapper markup around each individual paragraph instance. This is a heavy-handed approach that—as a themer and site builder—was frustrating to track down and override.

There is also a ton of markup in paragraphs-item.tpl.php, which I personally think is unwarranted, but at least the style and location of it is somewhat more standard, and thus easer for themers to manipulate.

Proposed resolution

Remove all markup from the theme_paragraphs_view() function.

Remaining tasks

None.

User interface changes

None.

API changes

None.

CommentFileSizeAuthor
#1 remove_wrapper_markup-2251909-1.patch449 bytesjstoller

Comments

jstoller’s picture

jstoller
he/him
commented
Status: Active » Needs review
StatusFileSize
new remove_wrapper_markup-2251909-1.patch449 bytes

Provide patch.

apmsooner’s picture

apmsooner commented

This patch works great for me.

jeroen.b’s picture

jeroen.b commented
Status: Needs review » Postponed (maintainer needs more info)

Why should I add this patch?
Overwriting the theme functions is possible in the theme, that's where you should remove the wrapper if you don't want it.

function MYTHEMENAME_paragraphs_view($variables) {
   $element = $variables['element'];
   return $element['#children'];
}

Adding wrappers is very common in Drupal and it's easy for site builders to style without touching code, that's why the wrapper is there.
The only thing I might add is a patch to move this theme functionality to a tpl file.

Also what about people that are currently using the wrapper to style their paragraph items, their styling will be messed up when they upgrade. Or what about people already overriding the theme function, they will have the problem that their overrides doesn't work anymore. So no API changes isn't really true.

jstoller’s picture

jstoller
he/him
commented
Status: Postponed (maintainer needs more info) » Active

While it’s true that this sort of wrapper markup is common in Drupal, it’s also true that much of the theming community has been bemoaning that fact for years now, seeing "divitis" as a disease to be cured. Rather than helping themers, this markup often gets in the way and hinders styling the page.

The Paragraph module is a special case, since it is intended to replace the body field. Every field in every paragraph bundle is already wrapped with markup by default. Further wrapping the paragraphs themselves is going to be unnecessary 90% of the time and the markup will be an obstacle in many, if not most, of those cases.

When I first installed this module, I overrode the paragraph-item.tpl.php, but that did nothing (I’m still not sure what that template is used for). Then I configured the paragraph field using Display Suite, removing all wrapper markup, but that didn’t work either. Then I started yelling at my computer, since I didn’t understand where the markup was coming from. Finally, I found the theme function and overrode it.

I suggest that, at a minimum, we move from a theme function to a template, to make it more accessible. I’d also make sure this was compatible with Display Suite, Fences, and other such modules that allow overriding this wrapper markup. I of course think the default template should just print the content, without adding any more markup, but if I can easily override that with Display Suite then I won’t care as much.

apmsooner’s picture

apmsooner commented

+1 to removing extra markup

jeroen.b’s picture

jeroen.b commented

@jstoller I understand. Maybe it's better to push something like that in the D8 version, not D7.
I will change the theme function into a template soon, but I'm not going to remove the extra wrapper just now.

theme_paragraphs_view = Wrapper around the whole paragraphs field output.
paragraph-item.tpl.php = Wrapper around every paragraph item (has suggestions with bundle/view mode)
Other wrappers around fields come from field.tpl.php I guess and the field theme functions.

jstoller’s picture

jstoller
he/him
commented

@jeroen.b, I found that theme_paragraphs_view() was wrapping each individual paragraph item. Not the whole paragraph field. And paragraph-item.tpl.php didn't seem to be doing anything last I checked, but it's possible that I was unintentionally overriding it elsewhere. I'll try playing with it again.

apmsooner’s picture

apmsooner commented

I've been able to use paragraph-item.tpl.php to remove 2 of the divs however there is still a div left from them_paragraphs_view() that as you mentioned would need to be overridden in template.php. Could we at the very least move that remaining div to paragraph-item.tpl.php also to simply overall theming for D7? I realize also its perhaps common practice to wrap the fields but i'm not sure i understand the necessity. My motivation is using the paragraph module in a document generation system thus the html is sent to pdf and the less html the better to speed up conversion.
Thanks

jstoller’s picture

jstoller
he/him
commented

After removing all my overrides, here's what I found. Looking from the inside out, each paragraph bundle is first wrapped in two divs from paragraphs-item.tpl.php. Then it is wrapped in another div from theme_paragraphs_view(). I've now confirmed that theme_paragraphs_view() does in fact wrap each item within a field and not the field as a whole. On top of all this, the standard Drupal field wrappers are applied. So each field item is wrapped again in a .field-item div, then the entire field is wrapped first in a .field-items div and then in a .field div.

All told, this means a paragraphs field is wrapped in two divs, and each bundle within it is wrapped in four divs. And this doesn't even include the three standard wrappers added to each field within the paragraphs bundles. I hope we can all agree that this is a little excessive.

At a minimum, we should...

  • Move all markup from theme_paragraphs_view() to paragraphs-view.tpl.php.
  • Ensure paragraphs-view.tpl.php is wrapping the entire paragraphs field as intended, rather than individual field items.
  • Remove the .clearfix class from paragraphs-view.tpl.php and paragraphs-item.tpl.php. Including this class is a design decision with a direct impact on layout. It should not be dictated by the module.
frans’s picture

frans commented

I don't see the issue here. Ofcourse Drupal is a DIV hell... but it always has been. It is easy to override the theme, and you should! Including empty theme functions or templates is useless imho. How can a themer know he/she can override that? Either the item wrapper is removed completly, or it outputs markup around the item.

The only fix I see is the removal of the .clearfix class. That is indeed a design decision I think.

jstoller’s picture

jstoller
he/him
commented

A themer's first instinct will be to look for templates. Providing templates, even if they don't do anything, lets themers know that they can theme the output of something. And documentation within the template should let them know what variables they have at their disposal.

Theme functions are significantly more obscure. They're harder to find, harder to interpret, and many less technical themers are afraid to mess with them at all because of the level of code involved. I would generally reserve theme functions for truly tricky bits that really need it.

As for Drupal being a div hell, that's only because developers keep putting the divs in there. It doesn't need to be that way. The fact other developers have used lots of unnecessary wrapper markup doesn't not excuse this module from doing the same. You've got to start changing things somewhere. And as I said before, I see this module as a special case, given its function, which makes the wrappers even less helpful and more harmful than usual.

The one other thing I'd suggest, since we're looking at changing things anyway, is that "paragraphs-view" is not terribly descriptive of the template's function. You might consider changing this to "paragraphs-items", or something along those lines. Then it will be very clear that paragraphs-item.tpl.php is for each individual item and paragraphs-items.tpl.php is for the whole group. That would certainly improve the themer experience.

  • Commit 3f56a98 on 7.x-1.x by jeroen.b: 
    This commit comes with quite some theme changes:
- Moved theme functions...
jeroen.b’s picture

jeroen.b commented
Status: Active » Needs review

Ok, after some thinking, I guess you guys are right. I just have some trouble changing this stuff while people are already using the module, but I did it anyway.

The commit above comes with the following changes:
- Moved theme functions to theme/paragraphs.theme.inc and paragraphs-item.tpl.php to theme/paragraphs-item.tpl.php
- theme_paragraphs_view has been removed in favor of the item below, as it made us have multiple wrappers, which was not intended.
- Added wrapper arround all paragraph items in one field:
- It has the following classes by default: paragraphs-items, paragraphs-items-view-mode-{view-mode}, paragraphs-items-field-{field_name}
- It has the following theme suggestions by default: paragraphs_items, paragraphs_items__{field_name}, paragraphs_items__{field_name}__{view_mode}
- It has its own default template: theme/paragraphs-items.tpl.php
- The template is made to look more like paragraphs-item.tpl.php rather than theme_paragraphs_view
- The templates also have proper comments now.
- I removed the clearfixes.

So... Please test and let me know, I think a new release is coming soon.

jeroen.b’s picture

jeroen.b commented

Hmm, now I wonder, should I have provided this as a patch first? ;-)

apmsooner’s picture

apmsooner commented

Works perfect for me :)

jeroen.b’s picture

jeroen.b commented

Great. Are there enough classes/theme suggestions by default or should I add more?

arbel’s picture

arbel commented

I would love to have the bundle type name on the wrapper div and not just on each indvidual item, can't find a way to do that....

jeroen.b’s picture

jeroen.b commented

Eh, the bundle name is dynamic, that's the whole point of this module? :-)

jeroen.b’s picture

jeroen.b commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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