Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Can we agree on a pattern to use to avoid the empty class attribute on tags if the class value is emptied out?
Like shown here if attributes.class
is empty.
<tag class="{{ attributes.class }}" {{ attributes }}>{{content}}</tag>
Fabian said in a previous issue to use something like this (but it looks complicated):
{{ attributes }}|merge({ class: ["my-class"] })
or use some Twig 'cleanup magic'.
Comments
Comment #1
markabur CreditAttribution: markabur commentedIn this case, wouldn't it be simplest to just go
<tag {{- attributes }}>{{ content }}</tag>
instead? Or does attributes.class *have* to be rendered separately from the other attributes?
Comment #2
anthonyR CreditAttribution: anthonyR commentedThis would seem logical yes. I guess at the moment the class is separate because it was like this in the original template files.
I don't know what this would mean for the Attribute() implementation that gets called in preprocess functions. Anyone?
Comment #3
markabur CreditAttribution: markabur commentedI think they would only need to be separate if the template is hard-coding a class, e.g.
Comment #4
anthonyR CreditAttribution: anthonyR commentedI'd love it if templates wouldn't hardcode classes but add them in preprocess_element() so they are easily wiped out if unwanted.
Comment #5
JohnAlbinThere is no use-case (or should be no use-case) in core for hard-coding class names. The only reason to pull out class separate from the rest of the attributes is to make it easier for new themers to add classes to an item.
New themer: “Oh, look! I can add a class right there.”
So the thing we need to support is this: have a very easy way for a new themer to discover how to add classes. Unfortunately, “adding them in preprocess_element” is not obvious or easy.
Comment #6
markabur CreditAttribution: markabur commentedThere is an issue open for that. :-D #1286530: Meta: Templates hard code class attribute, and shouldn't
But regardless of that issue, I still think it's ok/best to use the simpler form in templates that don't want to add any of their own classes (since it will print any classes that are in the attributes object anyway. So,
If the template needs to add a class such as clearfix, do this:
But if the template does not need to add any if its own classes, the simpler form is better:
Does that make sense as a rule?
Also see http://drupal.org/node/1727592 which suggests this pattern for phptemplate as well:
Comment #7
markabur CreditAttribution: markabur commentedI just started digging into twig this week and FWIW I was *extremely* confused by this pattern when I saw it in the work done so far, and in fact that's why I am in this issue now. I spent several hours trying to figure out why attributes.class was being treated specially, and never did find any discussion about it.
Comment #8
anthonyR CreditAttribution: anthonyR commentedI can understand that, would be better to discover. But if attributes.class is wiped we shouldn't be left with an empty class attribute on the tag, right?
So if our choice is to have attributes.class separately in the tag we should make sure that's not happening imo. We need a Twig pattern for this.
Comment #9
mortendk CreditAttribution: mortendk commentedAttributes & classes are not the same
attributes can be whatever we wanna add in fx data-foo="" so its a completely seperate thing than the classes (in some cases we can use the attributes for selection, but afaik its not always the case)
I Would rather do a simple condition in my twig file instead of the preprocess madness that we have right now.
Im annoyed by the class="" in the markup but that can be fixed in my tpl/twig file by testing if theres data in the {{class.whatever}} var
the pattern should be how can we do as much as possible inside the twig file instead of begging to hide it all out in template.php / modules this was what brought us into the mess we have to day :)
...sorry for the rant ;)
Comment #10
markabur CreditAttribution: markabur commentedAs a themer if I see either of the above in a template and I need a custom class on that div, I expect I will almost always do this, regardless:
Tada, no preprocess function needed, no messy template logic, and the excessive Drupal stuff I didn't want anyway goes away too. Win-win.
Are we saying it is ok to ship Drupal with it outputting
class=""
in some places?Comment #11
Fabianx CreditAttribution: Fabianx commentedHey, there here is Fabian and here is a new suggestion.
Most themers know JS, right?
What about we do this JQuery style?
Note: This is an hypothetical example, but is easy to implement in code and might also allow more - if we want.
The class="{{ attributes.class }} template-class" {{- attributes }} would continue to work.
I also think that if the chance is that attributes.class is empty that the pattern should not be used.
Comment #12
JohnAlbin/me blinks
That is an excellent idea! This is why I love open source discussions! I totally disagreed with Mark’s comment in #10 (since it would break any contrib modules’ JavaScript code that relied on a class that was added via PHP), but didn’t have an alternative approach to his or the current approach, so I kept my mouth shut. (Being critical without being constructive usually degrades the quality of the discussion.)
And now Fabian comes up with a solution that looks really friggin’ awesome. Nitpick: the twig syntax is wrong; dot operators are only used to traverse objects in twig. But how about we use Twig’s filters like this:
and/or:
How does that look to people?
Comment #13
Fabianx CreditAttribution: Fabianx commentedI agree that the filter syntax is much nicer, but that does not mean that the example is wrong. This would have been converted to:
So the function addClass of the object "attributes" would have been called with parameter x, but I agree that its wrong / weird for a "." notation to change something.
I like the generic add filter, but wonder if explicitly would not be better as people could try to use 'add' on strings as well and this is only for attributes, so even though this is longer, I'd propose:
to make it explicit and if we wanted to addClass could be calling addAttribute('class', 'x') internally.
( or maybe addAttr to be more jQuery like)
Note that we would have return addAttribute $this, so that the following would also work:
But, yeah that looks much nicer.
Comment #14
markabur CreditAttribution: markabur commentedThe nice thing about that is that is allows for removeAttribute too. Would it make sense to drop the extra "Attribute" and just make it add/remove? And/or append/prepend?
Which would leave us with something like the following once this issue is resolved (since the issue here is with templates that *don't* add classes):
The question is whether that's an improvement over this, which emits exactly the same thing:
I suppose the former is more discoverable -- as a themer who hasn't read a stitch of docs, I might guess that I add my classes as a space-separated string in between the single-quotes. With the latter I'd be more likely to ignore, intentionally or not, the supplied classes (but: only if I hadn't read any docs -- otherwise I'd pick up on the
attributes.class
syntax that we have today).Comment #15
steveoliver CreditAttribution: steveoliver commentedFrom #drupal-twig IRC:
Comment #16
jenlamptonWe talked about this in the Twig meeting this week, and we decided that we all prefer the existing syntax, the one we agreed upon back in March. Wherever attributes need to be added, they should be added like this:
First of all, it needs to be stupidly easy to add a class. That means that it needs to be easy for people who don't even know jQuerry. People need to be able to see a class="" tag, and insert their class right there. Without writing any code at all, even Twig code, even jQuery style code.
For those HTML purists who don't like the empty class tags, they can replace the code above in their template file with the following:
And guess what, the empty tag they don't like will go away.
yes, having empty class="" tags is messy, but it is far, far more important that Drupal is easy to understand, and easy to use, than that there are empty class tags in the markup. As long as there is a way for @mortendk to remove these ugly tags (which there is) then the existing patterns are acceptable.
Comment #17
markabur CreditAttribution: markabur commentedHow about chaining the verbs instead of nesting the parameters?
Comment #18
jenlamptonJust to comment about the adding of extra classes...
I'm hesitant about adding a third way to do exactly the same thing we can already do in 2 different ways. Part of the problem with the theme system today is that there are too many ways to achieve the same result, and people don't know when to do which.
If we need to allow people to add classes via a filter function in the Twig template, we can. But we should not be using that syntax in core (even in core themes) to add classes. Core modules should use preprocess to add their classes, and core themes should either use preprocess to add them, or add them directly into templates.
Weather we like it or not, people look at core themes as an "Example" of how to do things, people copy core themes and rename them, and they need to demonstrate *simple* examples of how to add a class.
Comment #19
carwin CreditAttribution: carwin commentededit: mostly removed because I misread @jenlampton's comment in #16
Not that it isn't technically valid but essentially what's being said in the example is: "I am a div, my identifier is " and leaving it at that.
Comment #20
markabur CreditAttribution: markabur commentedI can see doing this in a theme as an example for themers:
But do the core html-generating theme functions necessarily need to do it that way too?
Comment #21
jenlampton@carwin
No, writing good markup isn't a bad thing - but we can't do it at the expense of making Drupal hard to learn or hard to use. Adding a class to something is the *first* thing people want to do, and right now it is way, way, way to hard to do.
We have the opportunity to change that in D8. Let's make it easy for people to adopt Drupal! :)
Comment #22
jenlampton@markabur these are all template files, no more theme functions but I think yes.
Core also has a huge problem right now with consistency. We also have the chance to fix that in D8, so we should do everything the same way everywhere.
Think about the process for a front-end dev. 1) Locate a template file. 2) copy it to my theme. 3) add a class.
It doesn't matter where the template file came from, they still just need to add a class, and we need to make that easy for them.
Comment #23
markabur CreditAttribution: markabur commentedSo it sounds like all core twig templates should use
<tag class="{{ attributes.class }}" {{ attributes }}>
, regardless of whether there are any classes or attributes?Also, related: #1717186: Empty class attributes being printed in elements using $attributes, $title_attributes, or $content_attributes
Comment #24
markabur CreditAttribution: markabur commentedOne way to avoid outputting an empty class is to fill it with something.
Although this pattern makes it easier for themers by "propping open" the class attribute:
it can result in an empty class when you view source, e.g.
which is unattractive and has performance implications -- and, maybe more importantly, which is also impossible to search for. It's great that I'll be able to just copy a template to my theme, possibly name it something magical to limit its scope, and then add my own class to the div, but how do I discover the template to begin with?
If we always include a unique, findable classname inside of the template file, then it will never be empty, and when I view source I can easily figure out what file I need to copy to my theme. E.g.
Which looks like this when I view source:
Then I can search for "twig-html-block" and find the template file immediately.
Comment #25
Fabianx CreditAttribution: Fabianx commentedWe could do this in preprocess defaults or the twig engine display function, so its always available ... like:
I somehow like this suggestion. Not sure if more classes make something slower though.
It however is _definitely_ totally useful as debugging tool.
I think I'll add this to front-end unless there are huge objections to see how it would go?
We should also add comments in debugging mode:
Comment #26
markabur CreditAttribution: markabur commentedThe point about performance is just that lean HTML loads faster than fat HTML. If we were purely about speed, then we wouldn't be adding excess markup for themers -- but since we're not, I figure we might as well make the extra stuff as useful as possible and provide a little more markup for them.
For the class naming I had written it in "backwards template filename" format since that felt a bit more friendly/readable, but it's simpler and probably just as good to just go with forwards format. E.g. table.html.twig could use either
class="table-html-twig
orclass="twig-html-table
.Comment #27
jenlamptonI was told that the performance implications of an empty class tag is negligable (compared to everything else Drupal puts on the page, it is the least of our problems) but if that is not correct, then adding more CSS identifiers will make this problem worse, not better. Plus, it may anger the likes of @mortendk who think we have far too many classes already.
I, however, *love, love, love* this idea. If every template had a unique identifier (maybe something based on the theme hook suggestion?) then it would also be useful for figuring out when a template has been overridden.
Let's do it, and then benchmark to make sure that there really are no performance implications.
Comment #28
mortendk CreditAttribution: mortendk commentedThe problem about adding classes as we have done so far in drupal is that its basically wrong to write css as hooks using .block for blocks is adoption the hook system & that make sense for developers & engeneers but dosnt make sense in anyways for a FE (or the mindless .html class we have in the body)
Im all game for people of "lesser knowledge than the mighty frontend developers" (yes im talking bad about devs ;) ) need this, but pretty please lets have an option of killing this easy - Easy is NOT doing preprocess, its easy for a dev/those of us who have been dragged through drupal since 4.7 as a nessesary evil - cause its part of the developement workflow, but woudnt it be nice if we could actually make a theme without having to jump into sherlock holmes mode & just change the classes in the .twig file ?
The reason it angers & why we need to be able to get rid of the nonsense is basically to respect the craft of Frontend Development.
We cant keep having a system where a new FE comes ind and now suddenly have to accepts Drupals crazy ways of adding bs all over. Sorry but we have been doing it wrong for 8 years! (yes i have deep personal issues with this ;) ) So lets begin to embrase that this is not done for "developers" but for "themers/FED"
This make sense for anyone is plain easy to jump in and add a new class
What i would like to see as well was a way to kill the class that we dont need (but was added somehow from somewhere
fx killing of the node class
{{- attributes|kill('node') -}}"
A way to remove the empty class="" would then be a whole other operation that could be done as simple logic in the specific .twig file
by changing the way you printed the class - this would be a dodumentation issue & would be a thing that only the real obsessive themers would do ;)
pseudo code from hell:
The reason for this is that we need to chose somewhere who to please by doin it this way we
* make new FED happy -"yo easy to add n remove classses where i want them"
* gives the obsessive FED's a chance to clean up totally
Identifiers
Lets NOT add the identifier in as a class - its simply badcraftman ship & will cause more confusion - its industry standard to use comments as identifiers so lets for once try to do as the rest of the world does by adding this in as a comment - and a setting to remove would be the hot shit though :)
(*coughs* mothership does this allready but in a crappy way)
This would also remove the use of Develthemer that cause more confusion with its 48529 spantags all over & the crying from themers all over the world :)
Comment #29
JurriaanRoelofs CreditAttribution: JurriaanRoelofs commentedTotally agree with Morten here let's use this oppurtunity to cure the classitis.
Comment #30
anthonyR CreditAttribution: anthonyR commented+ 1 for making it easier (and more fun) to new front-end people and still have happy obsessive front-enders :)
Comment #31
fubhy CreditAttribution: fubhy commentedThat's a wonderful idea! +100
Comment #32
markabur CreditAttribution: markabur commentedIf Drupal outputs something like this:
Then there will be a bunch of bug reports about it, regardless of how well it is documented. What do we say to those, "works as designed, fix it yourself in your theme and BTW you are borderline OCD but we love you anyway"?
But if Drupal outputs something like this:
Then it's a documentation issue, about how to add a class. The fact that the tag is missing class is not in the least confusing to front-end developers. Then if a theme (core, contrib, or custom) wants to break out the class attribute in a template such as
theme-html-tag.html.twig
, that's fine, and that's the appropriate place for it too.So, we've been looking at this pattern:
Imagining that the newbie themer who has not read any Twig or Drupal theming docs will see that and understand that their custom classes should go either
"here {{
or}} here"
, and that they will also understand that the{{ attributes.class }}
part shouldn't be touched, even if it is emitting nothing (in case a contrib module later needs to add a class via PHP in order to do something with JavaScript).If that's what we are trying to accomplish, then we ought to provide better instruction inside of the template file itself, e.g.
To me it seems backwards/broken if the theme is having to remove cruft that comes from lower levels. Stuff that is necessary for theming should be added by a theme (frontend). Stuff that is necessary for identifying bits of content should be added by the backend.
Note that we currently have at least three open issues that talk about empty class being a bug...
Comment #33
steveoliver CreditAttribution: steveoliver commented@markabur: -1 to the idea of adding comments inline in tags.
Instead, I propose:
1. Theme debug mode, when enabled, produces
<!-- START 'path/to/template.html.twig' -->
and<!-- END 'path/to/template.html.twig' -->
comment tags.2. Theme documentation, which most everyone using D8 will need to read, will make it painfully clear how to add classes. Adding a class (or altering classes, as we've touched on many times in this thread), is a great example of how to work with
{{ attributes }}
that we can use throughout our various (to be created) theme tutorials and how-to's.Comment #34
jenlamptonIf we put the class we are talking about directly in the template Instead of preprocess), you won't need to preprocess or do anything complicated to remove it. Just delete it from the template. AND, if you are a HTML purist and don't like the empty class tag that gets printed, you can ALSO delete that from the template too, for example:
after Morten gets his hands on it, becomes:
Clean, easy and straightforward. I'm not sure I see a problem with the approach we decided on :)
Comment #35
steveoliver CreditAttribution: steveoliver commentedAlright, let's close this thing :)
Comment #36
steveoliver CreditAttribution: steveoliver commentedCreated follow-up issue: #1820158: Print template names in HTML comments.
Comment #37
mortendk CreditAttribution: mortendk commentedwow were agreeing on this *goddamn*
Comment #38
steveoliver CreditAttribution: steveoliver commentedI made the case(s) I could, but from #16:
Comment #39
crizThinking about this. And I am not sure if Drupal core module templates should output empty class attributes in any case, though I understand that this may be easier for beginners.
Drupal has some bad reputation for its html output anyway, this is making it only worse. Yes of course, those who know can get rid of this but the vast majority will not! So most sites will have empty class attributes in the markup, is this what we want to have? Also, people will not understand that they will have to take care about this, regardless how easy it will be.
So why don't we take care that Drupal core module templates provide the best markup possible? Core themes like stark can overwrite templates and do it like suggested above if needed. This way those who are new to frontend dev are going to be happy too (as core themes are a good starting point).
Comment #40
jenlamptonHaving no classes at all would make it very hard to handle everything we currently do with the CSS from core (I'm not talking about core themes, but from within core modules like system).
I'm sure some would argue that there shouldn't be any CSS either - but that's a whole other debate. I think this is a satisfactory decision for *now* but we can of course revisit after we get all the markup in core replaced. Incremental improvements++ :)
Comment #41
jenlamptonBetter title from @psynaptic.
Marked his issue as a dupe of this one:
#1817502: Standardize and simplify the attribute syntax in Twig template files
Comment #42
jwilson3I'll have to say I side with marcabur, steveoliver, criz, and David_Rothstein, on this one.
As a technical person, I want consistency and clean templates producing clean HTML. If there is an "easy way to add a class", then there should also be an easy way to remove a class, an id, or any other crazy attribute that Drupal provides ootb. Right? And I dont want to have to implement a preprocess function in the theme to remove classes... I should be able to do that by overriding the template, just like a beginner would, right?
From what I'm understanding from above, the proposal is to use the status quo (the one settled on in March) but then to be able to do what you want and remove extraneous classes, assumes that we should now now go through core preprocess functions and move all the classes that we can into the template file, and then it would be plainly obvious and easy to also remove the defaults.
However, this ignores the fact that there is still going to be some cases where we *have* to add classes (and other attributes) in preprocess functions -- we don't want to be using drupal_html_class() drupal_html_id() in twig files, or do we?
We just need and easy way to add and remove attributes, and use it everywhere, so its cleanly self-documented, and beginners will not have a problem.
I don't know how this:
Is any more confusing than this:
Or, if you're a one-liner neat-freak, this could all be condensed down to a single line, with some themer helper functions.
To conclude, I propose adding a set of self-documenting getter and setter functions like this:
If themers can't easily add to, edit, or remove the Attributes object contents before rendering, I fail to see the point of why we added the Attributes object in the first place.
Comment #43
Fabianx CreditAttribution: Fabianx commentedBack to active then, this is a good point for the engine.
Comment #44
jwilson3I put the proposal in #42 into a separate issue #1835396: Attribute modifier functions for themers to clear the slate and allow this to rest where it is.
Comment #45
Fabianx CreditAttribution: Fabianx commentedFollow-up in #44.
Comment #46
c4rl CreditAttribution: c4rl commentedPardon my tardiness to noticing this issue.
Question: Does the Stark vs Lush conversation at BADCamp have any bearing on these decisions?
To me it seems to follow that Stark ships with
While Bartik (Lush) ships with
Feel free to close again, but this seems worthwhile to discuss.
Comment #47
crizfor me #46 makes totally sense...
Comment #48
joelpittetI vote (albeit late):
<tag{{ attributes }}>
Splitting out the class
<tag class="{{ attributes.class }}"{{ attributes }}>
may be slightly easier for some themers but makes for messy code with tones of blankclass=""
in the rendered HTML markup, which would look like a mistake to most themers.Also if there is no attributes if you leave the space out the tag will be
<tag>
instead of<tag >
created with<tag {{ attributes }}>
Comment #49
markabur CreditAttribution: markabur commentedRe #46 -- that makes sense, to use both forms. I've been thinking that core's module templates in general ought to be succinct and elegant, but that certain themes could override those templates and show how to use the long form. And yes, Lush is what most new themers will look at first, so it's a good place to demonstrate patterns that they'll find useful going forward.
Comment #50
c4rl CreditAttribution: c4rl commentedWe were discussing this issue in IRC tonight and I will expand on #46.
As we discussed at the 2012 BADCamp Twig Sprint, we wish for core default markup (aka Stark) to be as clean and semantic as possible, while improving Bartik so that Bartik can be a viable base theme, more so than it is now.
The reason for this decision was to address two audiences of Drupal theming: Experts and Newcomers. Experts want extremely clean and semantic markup to start with as a base, while newcomers needs some guidance, suggestions, and examples. So, an expert themer might use Stark as a base theme, whereas a newcomer might use (an improved) Bartik as a base theme.
So, Stark should be as clean as possible, and Bartik should be easy to grok for people who are new. Ideally, then, we should relegate the literal usage
class
to improved Bartik templates (to be written).In this regard, here are our steps to proceed:
1. Get Twig templates converted as-is from Drupal 7 via #1757550: [Meta] Convert core theme functions to Twig templates. We need momentum and passing tests at this point for the sake of progress.
2. After we have some good RTBC patches, we will likely have other issues that focus on cleanup and consolidation of markup. Many of these will be stripping things out of core default markup. We've been using the "theme system cleanup" tag for many of these already.
3. We will create issues for core to move some conventions out of core default markup into Bartik. Bartik will likely ship with more templates than it did in Drupal 7 in this regard. For the sake of grok, these templates would have the literal
class="{{ attributes.class }}"
examples for newcomers to appreciate and understand. I believe that #1854344: [meta] Goals for core themes: Make Stark as semantic as possible; Make Bartik a better prospective base theme will serve as a starting point to organize this effort.Comment #51
psynaptic CreditAttribution: psynaptic commentedI would be very happy if Stark ships with the
<article{{ attributes }}>
syntax.Comment #52
joelpittetThis was fixed, just moving it to core