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

markabur’s picture

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>

In 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?

anthonyR’s picture

This 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?

markabur’s picture

I think they would only need to be separate if the template is hard-coding a class, e.g.

<tag class="{{ attributes.class }} clearfix" {{- attributes }}>{{ content }}</tag>
anthonyR’s picture

I'd love it if templates wouldn't hardcode classes but add them in preprocess_element() so they are easily wiped out if unwanted.

JohnAlbin’s picture

I'd love it if templates wouldn't hardcode classes but add them in preprocess_element() so they are easily wiped out if unwanted.

There 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.

<tag class="{{ attributes.class }}" {{- attributes }}>{{ content }}</tag>

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.

markabur’s picture

There 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:

<tag class="{{ attributes.class }} clearfix" {{- attributes }}>{{ content }}</tag>

But if the template does not need to add any if its own classes, the simpler form is better:

<tag {{- attributes }}>{{ content }}</tag>

Does that make sense as a rule?

Also see http://drupal.org/node/1727592 which suggests this pattern for phptemplate as well:

When upgrading themes, change:

<div class="<?php print $classes; ?>"<?php print $attributes; ?>>

to

<div class="<?php print $attributes['class']; ?> clearfix"<?php print $attributes; ?>>

[or simply]

<div <?php print $attributes; ?>>
markabur’s picture

<tag class="{{ attributes.class }}" {{- attributes }}>{{ content }}</tag>

New themer: “Oh, look! I can add a class right there.”

I 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.

anthonyR’s picture

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.

I 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.

mortendk’s picture

Attributes & 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 ;)

markabur’s picture

<div {{- attributes }}>{{ content }}</div>
<div class="{{ attributes.class }}" {{- attributes }}>{{ content }}</div>

As 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:

<div class="my-item-list">{{ content }}</div>

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?

Fabianx’s picture

Hey, there here is Fabian and here is a new suggestion.

Most themers know JS, right?

What about we do this JQuery style?

<div {{ attributes.addClass('x') }}></div>

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.

JohnAlbin’s picture

What about we do this JQuery style?

<div {{ attributes.addClass('x') }}></div>

/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:

<div {{ attributes|addClass('x') }}></div>

and/or:

<div {{ attributes|add('class', 'x') }}></div>

How does that look to people?

Fabianx’s picture

the twig syntax is wrong; dot operators are only used to traverse objects in twig.

I agree that the filter syntax is much nicer, but that does not mean that the example is wrong. This would have been converted to:

echo twig_render(getAttribute(getContextReference('attributes'), 'addClass', array('x')));

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:

<div {{ attributes|addAttribute('class', 'x') }}></div>

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:

attributes|addAttribute('class', 'x')|addAttribute('title', 'My accessibility title'|t)

But, yeah that looks much nicer.

markabur’s picture

<div {{ attributes|addAttribute('class', 'x') }}></div>

The 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?

<div {{- attributes|append('class', 'myclass') }}></div>

<div {{- attributes|remove('id') -}} id="myid"></div>

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):

<div {{- attributes|append('class', '') }}></div>

The question is whether that's an improvement over this, which emits exactly the same thing:

<div {{- attributes }}></div>

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).

steveoliver’s picture

From #drupal-twig IRC:

<steveoliver>: markabur++ :)
<steveoliver>: that's pretty powerful
<steveoliver>: a little cryptic, but something like that could be pretty awesome
     <carwin>: steveoliver: so I'm pretty new to twig, and you're right it is a little cryptic for me - am I correct in understanding the syntax in that as: <div {{- an_array|an_action_to_remove_from_that_array('to_remove') -}} ?
<steveoliver>: more like <div {{ object|methodAdd('to_array', 'element') }} >
<steveoliver>: where in this case to_array is 'class', and we're adding 'element' "myclass"
<steveoliver>: the syntax is a little muddy ATM, as multiple classes should probably be added more like <div {{ attributes|append('class', ['myclass1', 'myclass2']) }} >
     <carwin>: what about adding multiple attributes?
<steveoliver>: yeah, another similar issue
<steveoliver>: it almost feels like trying to do a little too much in one strike, as we kinda think through these use cases...
     <carwin>: attributes|append(('class', [class1], [class2]),('placeholder', 'excellent placeholder here'))
<steveoliver>: ...there ya go
<steveoliver>: that don't seem so bad :)
     <carwin>: so where in d8 would attributes initially defined? i'm guessing there will be sort of a stock set.
<steveoliver>: in the call to the theme() function for whatever output is being produced.
     <carwin>: gotcha
     <carwin>: so the 'remove' in your second example is a method that extends the append method then
     <carwin>: i see
     <carwin>: and if no id is specified it just removes all ids
<steveoliver>: not sure about extends... but is the opposite
<steveoliver>: and <div {{- attributes|remove('id') -}} id="myid"></div> would just wipe out ids and then hard code the id="" attribute.
     <carwin>: what's the purpose of leaving an empty attribute in the markup?
<steveoliver>: it wouldn't necessarily be an empty attribute...
<steveoliver>: there would be attributes.class left, etc.
<steveoliver>: i.e. "all attributes besides 'id' + my hard-coded id".
<steveoliver>: that's the second one
<steveoliver>: "attributes|append(('class', [class1], [class2]),('placeholder', 'excellent placeholder here'))" I really like that example
<steveoliver>: super slick
<steveoliver>: not sure how friendly to non-coders, but... :)
     <carwin>: ok I think I understand - but let me double check with a sort of modification of the second example: <div {{- attributes|remove('id') -}}></div> would output: <div id=""></div> ?
     <carwin>: steveoliver: thanks buddy :)
     <carwin>: I think non-coders at some point will have to at least learn to understand the order of operations :P
<steveoliver>: exactly.  it's like "Hey, you're in here, aren't you.  Figure it out." :)
<steveoliver>: not necessarily <div id=""></div> ... more like <div class="class1 class2"></div> instead of <div class="class1 class2" id="this-id"></div>
<steveoliver>: it's just {% unset attributes.id %} ... {{ attributes }}
     <carwin>: cool, I was worried about just leaving blank attributes after removing their values
     <carwin>: in the markup
<steveoliver>: right, which is what [this] issue was kinda about...
<steveoliver>: i think attributes|remove('id') should be shorthand for {% unset attributes.id %} {{ attributes }}
jenlampton’s picture

We 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:

<div class="{{ attributes.class }}" {{ attributes }}>

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:

<div {{ attributes }}>

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.

markabur’s picture

attributes|append(('class', [class1], [class2]),('placeholder', 'excellent placeholder here'))

How about chaining the verbs instead of nesting the parameters?

attributes|append('class', [class1], [class2])|append('placeholder', 'excellent placeholder here')
jenlampton’s picture

Just 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.

carwin’s picture

edit: 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.

markabur’s picture

I can see doing this in a theme as an example for themers:

<div class="{{ attributes.class }}" {{ attributes }}>

But do the core html-generating theme functions necessarily need to do it that way too?

jenlampton’s picture

@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! :)

jenlampton’s picture

@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.

markabur’s picture

So 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

markabur’s picture

One 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:

<div class="{{ attributes.class }}" {{ attributes }}>

it can result in an empty class when you view source, e.g.

<div class="">

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.

<div class="twig-html-block {{ attributes.class }}" {{ attributes }}>

Which looks like this when I view source:

<div class="twig-html-block">

Then I can search for "twig-html-block" and find the template file immediately.

Fabianx’s picture

Priority: Normal » Major

We could do this in preprocess defaults or the twig engine display function, so its always available ... like:

$variables['attributes']['class'][]=convert_to_css_class_name_somehow($name);
return ...->load($name)->render($variables);

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?

Fabianx: <table class="table-html-twig">...</table>
[4:02pm] Fabianx: etc.
[4:02pm] Fabianx: That would make finding the templates a piece of cake ...
[4:02pm] Fabianx: And could even work with suggestions:
[4:02pm] Fabianx: <table class="table-html-twig table---node-1-html-twig">...</table>
[4:03pm] Fabianx: And you would _always_ know what it is that you are theming how 
[4:03pm] Fabianx: Directly from markup.
[4:03pm] Fabianx: ( for debugging )

We should also add comments in debugging mode:

<!-- START node--1.html.twig -->
<!-- END node--1.html.twig -->
markabur’s picture

The 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 or class="twig-html-table.

jenlampton’s picture

I 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.

mortendk’s picture

The 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

<article class="myclassthatmakesense {{ attributes.class }}" {{ attributes }}>

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') -}}"

<article class="myclassthatmakesense {{ attributes.node.class }} {{attributes.node|kill('node') -}}" {{ attributes }}>

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:

if attributes.node != "" {
   attributes.class = 'class="{{ attributes.node.class }} " '
}
<article {{ attributes.class }}>....

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)

<!-- START where-is-this-crap-comingfromt.twig -->
<article>
bla bla 
</article>
<!-- END where-is-this-crap-comingfromt.twig -->

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 :)

JurriaanRoelofs’s picture

Totally agree with Morten here let's use this oppurtunity to cure the classitis.

anthonyR’s picture

+ 1 for making it easier (and more fun) to new front-end people and still have happy obsessive front-enders :)

fubhy’s picture

That's a wonderful idea! +100

markabur’s picture

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 ;)

If Drupal outputs something like this:

<article class="">

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:

<article>

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:

<tag class="{{ attributes.class }}" {{- attributes }}>{{ content }}</tag>

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.

<tag {# To add a class, uncomment the following and insert the class after the opening quotation mark: #} {#class="{{ attributes.class }}"#} {{- attributes }}>{{ content }}</tag>

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...

steveoliver’s picture

@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.

jenlampton’s picture

If 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:

<textarea class="form-textarea {{ attributes.class }}" {{- attributes }}>{{ value }}</textarea>

after Morten gets his hands on it, becomes:

<textarea {{- attributes }}>{{ value }}</textarea>

Clean, easy and straightforward. I'm not sure I see a problem with the approach we decided on :)

steveoliver’s picture

Status: Active » Closed (works as designed)

Alright, let's close this thing :)

steveoliver’s picture

mortendk’s picture

wow were agreeing on this *goddamn*

steveoliver’s picture

I made the case(s) I could, but from #16:

We 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.

...apparently I missed out on some agreement and haven't had any support on my proposals (RTFM + comments pointing you to template files), so I said "whatever"...

I expect we'll revisit this post feature freeze.

criz’s picture

Thinking 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).

jenlampton’s picture

Having 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++ :)

jenlampton’s picture

Title: Agree on pattern to avoid having empty class attribute » Standardize and simplify the attribute syntax in Twig template files

Better title from @psynaptic.
Marked his issue as a dupe of this one:
#1817502: Standardize and simplify the attribute syntax in Twig template files

jwilson3’s picture

I'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:

{# Add a custom class, and remove zebra striping, we're using css3 #}
{% attributes.class|add('custom-class') %}
{% attributes.class|remove('odd','even') %}
<tag{{ attributes }}>

Is any more confusing than this:

// in template.php
function mytheme_sometemplate_preprocess(&$vars) {
 // this probably isnt the right code... but anyway
 unset($vars['attributes']->class['odd']);
 unset($vars['attributes']->class['even']);
}
// in templates/sometemplate.twig
<tag class="custom-class {{ attributes.class }}" {{- attributes }}>

Or, if you're a one-liner neat-freak, this could all be condensed down to a single line, with some themer helper functions.

<tag{{ attributes|addClass('custom-class')|removeClass('odd','even') }}>

To conclude, I propose adding a set of self-documenting getter and setter functions like this:

attributes.class|add(classname1, classname2, ...)
attributes.class|remove(classname1, classname2, ...)
attributes|setId(id)
attributes|addClass(classname1, classname2, ...)
attributes|removeClass(classname1, classname2, ...)
attributes|add(attributename,value)
attributes|remove(attributename)
...

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.

Fabianx’s picture

Component: Twig templates » Twig engine
Status: Closed (works as designed) » Active

Back to active then, this is a good point for the engine.

jwilson3’s picture

I 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.

Fabianx’s picture

Component: Twig engine » Twig templates
Status: Active » Closed (works as designed)

Follow-up in #44.

c4rl’s picture

Pardon 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

<article{{ attributes }}>

While Bartik (Lush) ships with

<article class="node {{attributes.class}}" {{- attributes }}>

Feel free to close again, but this seems worthwhile to discuss.

criz’s picture

Component: Twig templates » Twig templates conversion (front-end branch)
Status: Closed (works as designed) » Active

for me #46 makes totally sense...

joelpittet’s picture

I 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 blank class="" 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 }}>

markabur’s picture

Re #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.

c4rl’s picture

Status: Active » Postponed

We 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.

psynaptic’s picture

I would be very happy if Stark ships with the <article{{ attributes }}> syntax.

joelpittet’s picture

Project: » Drupal core
Version: » 8.0.x-dev
Component: Twig templates conversion (front-end branch) » ajax system
Issue summary: View changes
Status: Postponed » Fixed
Issue tags: +Twig

This was fixed, just moving it to core

Status: Fixed » Closed (fixed)

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