node.html.twig template

Remove ID attribute or make it a class instead

We should remove the node-[nid] id. IDs need to be unique and I don't think we can safely assume a given node will only appear once on a given page. It's feasible that a single node may show up in multiple views on a single page or it may be shown more than once using different view modes.

If there is no specific functionality dependent on this id, we should remove it completely or move it into the class array via preprocess.

Remove clearfix class

There is no layout CSS attached to the node that I know of that would require clearfixing. If a theme needs to create a multi-column layout within a node, they can add a clearfix class then. If we do need to keep it, is there a reason we want this hardcoded in the template rather than adding it to the classes array?

Remove comment hiding comment?

{# We hide the comments and links now so that we can render them later. #}
Is that comment relevant anymore? I don't see where we are hiding comments.

#2: All good points. Some additional questions:

- Is the ‘submitted’ class the best name for this? Can we omit the class entirely?
- It looks like {{content.field_x}} and {{content.remainder}} are an example; in the actual template would this simply be {{content}}?

i have looked at this now and tried to change the css classes so that they match the CSS Coding Standards:

attached is a patch to

• Remove ID attribute and make it a class instead, see #2
• Remove clearfix class , see #2
• Use node_submitted instead of submitted according to the css coding standards, see #3
• Reorder the hiding logic of links and comments to match their order of use

regarding to "Remove comment hiding comment" in #2:
as of the code, we are still hiding those elements.

core node.html.twig vs. bartik node.html.twig

bartik currently is using its own node template. i have looked at the difference (see the second attached file).
does this really make sense? i have the feeling, that the current implementation is more about inconsistencies rather than added value for the bartik theme.
we might want to put this discussion in a separate issue.

removed the comment related to the id attribute

It looks like {{content.field_x}} and {{content.remainder}} are an example; in the actual template would this simply be {{content}}?

Yes, {{ content.field_x }} was an example, but in the actual template it would be this:

  <div{{ content.attributes }}>
    {{ content.remainder }}
  </div>

this will allow teme developers to change or remove the div tag around the content directly in the node template. Printing {{ content }} will include the surrounding div tag.

regarding to "Remove comment hiding comment" in #2:
as of the code, we are still hiding those elements.

We'll need to clean up the way comments and links are added (in node_view? in preorocess?). They should not be "inside" content in the first place, and when we get them out of there we won't need to hide anything anymore. It would be great to see that in this patch as well.

For differnces between core node.html.twig and bartik's node.html.twig:

1. we should not use template inheritance in core, but it's there if conrib wants to use it
2. there may not be very many differences between these templates right now, but tat's okay. We'll be cleaning all the yuck out of core and moving it into bartik (and hopefully changing it to yum in the process) so by the time we ship there will be a lot of differences.

Thanks for the feedback!

i have updated the patch with some attempts:

• use separate variables for comments & links in node twig (comments seem to be working, but links aren't tested at all), see #7
• update the multilingual field test case to reflect the class name changes, see test failure in #5 (this isn't finished and tests are expected to fail again)

i have introduced comment_preprocess_node as i think this is the only way to expose comments as separate variables in a sane way.

    // Only append comments when we are building a node on its own node detail
    // page. We compare $node and $page_node to ensure that comments are not
    // appended to other nodes shown on the page, for example a node_reference
    // displayed in 'full' view mode within another node.
    if ($node->comment && $view_mode == 'full' && node_is_page($node) && empty($node->in_preview)) {
      $node->comments = comment_node_page_additions($node);
    }

the above code related to rendering the comments for a node still remains in comment_node_view, but maybe we want to move this over to the preprocess function?

@jenlampton Thanks for clarifying with #7.

With this:

@@ -75,14 +75,10 @@
+<article class="node--{{ node.nid }} {{ attributes.class }}"{{ attributes }}>

Would attributes.class output additional classes by default? I ask because node--{{ node.nid }} is using the “component variant” naming convention, which means we’d need a 'node' class too: class="node node--{{ node.nid }} …". I wonder if we even need the node id at all by default. A minimal version could be just class="node node--{{ node.type }}".

Would attributes.class output additional classes by default?

Yes. I believe node and node--[type] as well as node--published and maybe even node--sticky are all added there by default. If there isn't anything in core that needs a node-[nid] class added, can we just move that to Bartik?

edit:
here are the classes I get from stark (but it's currently the same in bartik):

node node-article promoted view-mode-teaser contextual-region clearfix

The goal here is to make core as clean as we can, and move anything base-theme-like into Barrik, as per #1854344: [meta] Goals for core themes: Make Stark as semantic as possible; Make Bartik a better prospective base theme

Additionally {{ node.nid }} should not be used. It should have been {{ nid }} as per the issue summary ;)

@jenlampton Sounds good then! I guess we’ll need to check if anything needs the nid in core, but if not, this can absolutely get moved to Bartik. One less default class :)

Giving it another pass. New changes include:

• {{ node.nid }} changed to {{ nid }} and moved to Bartik (but we should confirm this doesn't break anything)
• added @todo links to the trans issue for the slaughter of {{ submitted }}
• renamed {{ user_picture }} to {{ author }}
• renamed {{ node_url }} to {{ url }} - in node module
• renamed {{ node_url }} to {{ url }} - in RDF module

I also created a new issue for the title & attributes cleanup: #2004966: Markup and variable cleanup for titles and attributes in all templates

I searched core for the following strings and found nothing referencing the node id from the markup:
- .node-
- #node-
- getElementById('node
- getElementById("node

#13 is looking really great. Some details:

+++ b/core/modules/node/templates/node.html.twigundefined
@@ -75,38 +76,31 @@
+      <p class="node__submitted">{{ submitted }}</p>

Let’s remove the <p> wrapper around {{ submitted }}. Bartik’s node template doesn’t have it.

+++ b/core/themes/bartik/templates/node.html.twigundefined
@@ -78,38 +79,35 @@
+<article class="node__{{ nid }} {{ attributes.class}} clearfix"{{ attributes }} role="article">

class="node__{{ nid }} should be class="node--{{ nid }}. The double-underscore denotes a child element; the double-dash is a modifier class (special-casing the 'node' class). Because if this, I’d also like the modifier to come after the default classes: <article class="{{ attributes.class }} node--{{ nid }} clearfix"….

remainder change

updated as per recent comment

@ry5n thanks!

I've updated the issue summary to reflect your changes in #14, and updated the comment issue to match:
#2031883: Markup for: comment.html.twig

add link to comment

blocked

Bot?

I just looked over the code...

@mortendk this is going to be just way easier for folks to understand. Great job!

Testbot?

Tagging novice: Someone needs to fix the syntax error in node.module.

remove comment, add view mode

#1927584-118: Add support for the Twig {% trans %} tag extension was committed!!

Issue is now blocked by: #2047095: Remove $submitted from node templates

Updated issue summary.

There is now a patch in #2047095: Remove $submitted from node templates

Rerolling patch from #18 and fixing syntax error.

changing status.

Updated issue summary.

Testbot, get to work!

Ok, sorry guys. This isn't isn't blocked by #2047095: Remove $submitted from node templates (read comment I made there on why). It's now a dup of this issue.

Here's a review:

1. +++ b/core/modules/comment/comment.module
   @@ -621,7 +621,7 @@ function comment_node_view(EntityInterface $node, EntityDisplay $display, $view_
   +      $node->comments = comment_node_page_additions($node);
   
   @@ -1462,6 +1462,13 @@ function comment_preview(Comment $comment) {
   + * Implements hook_preprocess_HOOK() for node.html.twig.
   + */
   +function comment_preprocess_node(&$variables) {
   +  $variables['comments'] = $variables['node']->comments;
   +}
   +
   +/**
   

   I'm confused. Aren't these the same thing? Why can't we just use {{ node.comments }} in the template file? Idk, just seems redundant.

2. +++ b/core/modules/node/node.module
   @@ -717,6 +719,7 @@ function template_preprocess_node(&$variables) {
   +   // @todo remove submitted once http://drupal.org/node/1927584 is resolved.
        $variables['submitted'] = t('Submitted by !username on !datetime', array('!username' => $variables['name'], '!datetime' => $variables['date']));
   
   @@ -725,13 +728,18 @@ function template_preprocess_node(&$variables) {
   +   // @todo remove submitted once http://drupal.org/node/1927584 is resolved.
        $variables['submitted'] = '';
   

   These need to be removed.

3. +++ b/core/modules/node/node.module
   @@ -759,7 +767,7 @@ function template_preprocess_node(&$variables) {
   +  // @todo these should be attributes on content instead.
      $variables['content_attributes']['class'][] = 'content';
   

   I think these should be, yes. Just not sure how this will actually impact {{ content }}.

4. +++ b/core/modules/node/templates/node.html.twig
   @@ -6,6 +6,7 @@
     * Available variables:
     * - node: Full node entity.
     *   - type: The type of the node, for example, "page" or "article".
   + *   - nid: The node ID of the node.
     *   - uid: The user ID of the node author.
     *   - created: Formatted creation date. Preprocess functions can reformat it by
     *     calling format_date() with the desired parameters on
   

   It's not in here, but the - submitted variable needs to be taken out too.

5. +++ b/core/modules/node/templates/node.html.twig
   @@ -81,31 +82,28 @@
   -<article id="node-{{ node.nid }}" class="{{ attributes.class }} clearfix"{{ attributes }}>
   +<article class="{{ attributes.class}}" {{ attributes }} role="article">
   

   There's no real need to separate the classes from the attributes. This is generally only done if we're manually adding a class via the template (ie: like clearfix... which we seem to be taking out?) or something. Since we're not, it should just be: <article{{ attributes }} role="article">

6. +++ b/core/modules/node/templates/node.html.twig
   @@ -81,31 +82,28 @@
      {{ title_prefix }}
      {% if not page %}
        <h2{{ title_attributes }}>
   -      <a href="{{ node_url }}" rel="bookmark">{{ label }}</a>
   +      <a href="{{ url }}" rel="bookmark">{{ label }}</a>
        </h2>
      {% endif %}
      {{ title_suffix }}
   

   See my comment in #1982244-26: Markup for: block module about the title stuff... we should probably change this to match.

7. +++ b/core/modules/node/templates/node.html.twig
   @@ -81,31 +82,28 @@
      {% if display_submitted %}
        <footer>
   -      {{ user_picture }}
   -      <p class="submitted">{{ submitted }}</p>
   +     {{ author }}
   +     <div class="node--submitted">{{ submitted }}</div> {# @todo: split the data up ? #}
        </footer>
      {% endif %}
   

   Why are we using <footer/> here?? Shouldn't this be <header/> instead? After discussing this with @mortendk on irc, the W3 footer spec, says the author and date stuff should be in <footer/>

   Might want to put the title in <header/> too... idk, thoughts? edit: see Bartik's node.html.twig file.

   The submitted stuff needs to be expanded out using the new {% trans %} tag. See the latest patch in #2047095: Remove $submitted from node templates.

8. +++ b/core/modules/node/templates/node.html.twig
   @@ -81,31 +82,28 @@
   -  <div{{ content_attributes }}>
   -    {# We hide the comments and links now so that we can render them later. #}
   -    {% hide(content.comments) %}
   -    {% hide(content.links) %}
   +  <div class="content"> {# @todo: do we want .content ? #}
        {{ content }}
      </div>
   

   This should probably be {{ content.attributes }}... but... idk, wouldn't that mean we would have to make the actual content {{ content.content }}? That seems messed up, thoughts?

9. +++ b/core/modules/node/templates/node.html.twig
   @@ -81,31 +82,28 @@
   -  {{ content.links }}
   -  {{ content.comments }}
   +  {{ links }}
   +  {{ comments }}
   

   I think we can get away with just {{ node.comments }} here. Or do we really, really want to have comments be a TLV (top level variable)?

10. +++ b/core/themes/bartik/templates/node.html.twig
    @@ -26,12 +27,12 @@
      * - submitted: Submission information created from name and date during
      *   template_preprocess_node().
    

    Take this out.

11. +++ b/core/themes/bartik/templates/node.html.twig
    @@ -77,38 +78,35 @@
         {{ title_prefix }}
         {% if not page %}
           <h2{{ title_attributes }}>
    -        <a href="{{ node_url }}">{{ label }}</a>
    +        <a href="{{ url }}">{{ label }}</a>
           </h2>
         {% endif %}
         {{ title_suffix }}
    

    Same "drillable" idea here as above.

12. +++ b/core/themes/bartik/templates/node.html.twig
    @@ -77,38 +78,35 @@
         {% if display_submitted %}
           <div class="meta submitted">
    -        {{ user_picture }}
    +        {{ author }}
             {{ submitted }}
           </div>
         {% endif %}
    

    This just needs to be refactored using the new {% trans %} tag as above. EDIT:This should also be changed from <header/> to <footer/>. After discussing this with @mortendk on irc, the W3 footer spec, says the author and date stuff should be in <footer/>.

Ps... can we also PLEASE remove .title from the <h2/> tag??? There should only be one <h2/> per node anyway, it's really annoying.

yes we can kill it right now kill kill kill kill kill kill kill

Updated issue summary.

Actually there is a good reason to separate class attributes - to make it easy for themers to add classes without preprocess function or knowing they can break out attributes in this way. Node, page, comment and block are very likely candidates for this sort of class adding.

Removing .title selector from h2 goes against our own coding standards - so should be .node--title or something like that, we should not have to do stupid descendant/child selector to style a node title, .node > h2 {} is uncool in anyones book, we should use .node--title {}, which is scalable/more robust, aka what if later some tricky dev adds a new inner wrapper to node, then child selector is broken, uh oh, we relied on html structure for presentation, bummer for you.

You can say "should have one h2 per node", not quite right and not best practice in html5, if I section, say on fields, then html5 headings reset, I can have as many h2's as I want in as many sections as I want. We can't assume how html5 is going to be used by the dev, or in common practice in 3/4 years time.

we can add .title whatever into bartik but not in stark - thats not going against coding standards & thats what have been disucssed the last year to have a drupal core that is clean of " as much as possible" (https://drupal.org/node/2008464#principles)

if you want that kinda classes like h2.title then they should be added into bartik, thats gonna be turned into a prober basetheme.

OK, I see those docs now, they seem a bit at odds with the CSS standards, hence some confusion, but yeah, I am pretty keen about breaking out those attributes, that looks to be within scope of the principles aka 'visibility within templates' etc. Don't you mean discussed for the last 8 years? ;)

And yeah, I am all for getting rid of as many generic classes like title, content etc as possible.

node.twig.html -> node.html.twig

Add related follow up

Rerolled patch.
Applied changes from comment #28.
Function comment_node_view is not more exists - should we add it back?

1. +++ b/core/modules/comment/comment.module
   @@ -1326,6 +1326,13 @@ function comment_preview(CommentInterface $comment, array &$form_state) {
   + * Implements hook_preprocess_HOOK() for node.html.twig.
   + */
   +function comment_preprocess_node(&$variables) {
   +  $variables['comments'] = $variables['node']->comments;
   +}
   +
   +/**
   

   I don't think this is needed.

2. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeFieldMultilingualTestCase.php
   @@ -132,9 +132,9 @@ function testMultilingualDisplaySettings() {
   +      ':node-class' => 'node-.' . $node->nid,
   

   Added a . to the node-class by accident.

3. +++ b/core/themes/bartik/templates/node.html.twig
   @@ -96,7 +96,7 @@
      {% if content.links %}
        <footer class="link-wrapper">
   -      {{ content.links }}
   

   This is strange we are checking for content.links and printing links.

+++ b/core/modules/node/templates/node.html.twig
@@ -75,20 +74,26 @@
+      <a href="{{ url }}" rel="bookmark">{{ label }}</a>

Why not node.url ?? I see no reason to add a custom var. It isn't consistently.

Couple more thing, and I'm with @droplet on the node.url thing it seems to have more context but curious your thoughts there as it's a bit subjective:

Also could you provide an interdiff with the changes?

1. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeFieldMultilingualTestCase.php
   +++ b/core/modules/node/lib/Drupal/node/Tests/NodeFieldMultilingualTestCase.php
   @@ -132,9 +132,9 @@ function testMultilingualDisplaySettings() {
   +      ':node-class' => 'node-' . $node->nid,
   

   This should be $node->id() still.

2. +++ b/core/themes/bartik/templates/node.html.twig
   @@ -71,23 +71,16 @@
   +<article class="node--{{ node.id }} {{ attributes.class}}" {{ attributes }} role="article">
   

   Missing a space after attributes.class inside the {{ }}.
   And the {{ attributes }} needs the space before it remove because it prints a space automatically before each attribute.

Applied suggestions from comment #40.
Uploaded interdiff.txt against #25.

Thanks for the interdiff @babruix! On the classes need space I think you went the other way and remove the space. Should look like this, like all the rest of them:
{{ attributes.class }}

+++ b/core/themes/bartik/templates/node.html.twig
@@ -71,23 +71,16 @@
+

And could you remove extra space after it as well?

Not really a feature IMO :)

Tagging for focus.

Will check this one...

Tested locally, all tests passed.
Bot retest...

45: node-2004252-45.patch queued for re-testing.

+++ b/core/modules/node/templates/node.html.twig
@@ -75,20 +74,26 @@
+  {% if content.links %}
+    <footer class="link-wrapper">
+      {{ author }}
+      {% if display_submitted %}
+        <div class="node--submitted">
+          {{ submitted }}
+        </div>
+      {% endif %}
+
+      {{ content.links }}
     </footer>
   {% endif %}
 

i can't see a reason why author + submitted should go into the link-wrapper & why they should only appear when content.links is set

We had discussion (@dasjo, @rteijeiro, @LewisNyman and me) at Drupal Developer Days in Szeged and agreed this patch need few more improvement to improve logic and D8 CSS coding standard. I will make these improvements...

+++ b/core/themes/bartik/templates/node.html.twig
@@ -71,23 +71,16 @@
+<article class="node--{{ node.id }} {{ attributes.class }}"{{ attributes }} role="article">

the role is allready added in the preprocess

if theres concencus about moving out all the classname generating of the preprocess #2217731: Move field classes out of preprocess and into templates that would be another thing to consider maybe getting into this patch as well (or as a follow up, to get the ball rolling)

1. +++ b/core/modules/node/templates/node.html.twig
   @@ -75,20 +74,26 @@
   +<article class="node--{{ node.id }} {{ attributes.class }}"{{ attributes }} role="article">
   

   I don't think we would need a double dash here. Just one dash is fine. Maybe node-id-{{ node.id }}

2. +++ b/core/themes/bartik/templates/node.html.twig
   @@ -96,6 +89,12 @@
   +        <div class="node--submitted">
   

   I think we discussed already to change this to underscores

i'd be fine with node--ID as from my perspective that's a modifier class.
https://drupal.org/node/1887918#extend

Changes done in this patch:

• Improved logic for {% if content.links or author or display_submitted %}. (in node module and bartik theme)
• Removed class="link-wrapper" from footer. (in node module and bartik theme)
• Changed class class="node--submitted" to use underscores. (in node module and bartik theme)
• Changed node ID in class class="node--{{ node.id }}" to use double dashes. (in node module and bartik theme)

Node IDs are not removed because some test require them. For this I suggest follow up task, which will cover tests also.

This patch does not pass tests, I tried to fix them too but after spending too much time I give up...

I re-rolled this as it would not apply cleanly. The tests that failed in #45 work locally. I also made the spacing consistent between the content.links the module and bartik files.

I corrected the XPath syntax to reflect the current HTML structure of the body tag.

Changing Status to review.....

I've addressed the above to comments in 65. Somewhere since the patch in 25 we lost that url variable in template_preprocess_node; I also fixed the missing documentation error.

• get rid of node_url
• get rid of role="article" in core node template
• get rid of role="article" in Bartik node template

+++ b/core/modules/node/templates/node.html.twig
@@ -75,20 +76,25 @@
-<article id="node-{{ node.id }}" class="{{ attributes.class }} clearfix"{{ attributes }}>

why are we keeping id="node-{{ node.id }}" ?
cant we get rid of that once & for all :)

Besides of that it looks good to me, think if we need some screenshots for bartik, seven & stark

@mortendk: Check @Dragan's comment #2004252-58: node.html.twig template. Some tests need node id so if we get rid of it, tests fail.

rename the .content to .node--content, to keep up with our css standard.

I have tried to change the test, so it goes after the article class="node--$id instead of article ID="node--$id

As far as I've noticed there is "big" mess around Drupal 8 because making decisions per task. Guess "id" attributes are not removed from all twig files, but we want to remove them? Why not creating new META issue with related issues per twig file, so we can make it unified and standardized avoid to miss it somewhere?

removing all of the id's in one big giant patch will become a cluster F. - We take em one by one, template by template & optimize on each element as we go, remoiving them all in one would guarantee that we would end up not beeing able to do anything.

But yes it would be nice to be able just to clean out all the cruft in one go ;)

For what it's worth my buddy and I beat on trying to get that Multilingual test to pass using the class name instead of the ID for like 2 hours at the Chicago sprint and we had no such luck. Seems like it should totally work, but I'd love to see it work if someone knows XPath syntax better than I...

@mortendk no one giant patch... Separate patch for each task... But one "META task" for all smaller tasks.

I dont think it would be a productive idea to create a new task every time a test fails, we "just" have to dig in and battle the test monster or find someone whos better at writing tests, then we can focus on the markup / classes :)

id's changed to classes for testbot ...

New to this issue, but want to lend a hand. Here's a petty attempt to fend off the test bots...
node title test was missing a dash on the node--ID class (i think)

I've checked the other failing tests... they look fine to me, perhaps we are missing something on the twig file?

test is updated, so we dont need the id="" anymore :)

+++ b/core/modules/node/node.module
@@ -669,15 +667,15 @@ function template_preprocess_node(&$variables) {

If we use hidden formatter for title in node display then template_preprocess_node shows a warning. Can we fix it here? It only requires simple isset check.

patch updated as per #84.

It looks that something happened with "submitted" content. Take a look at the screenshots:

Before

After

ok im on it & gonna put in the screenshots as well

seperated out the links & moved them above the comments, the "add a comment" link was placed belov the comment form - which makes no sense ;)

fixed the submitted issue as mentioned in #86

Heres the screenshots for patch #88

Bartik

Bartik teaser before

Bartik teaser after

Bartik node before

Bartik node after

Stark

Stark teaser before

Stark teaser after

Stark node before

Stark node after

forgot to fix the classes to follow our own css naming standard (https://drupal.org/node/1887918#sub-components)
no changes to screenshots *phew*

updated the issue with proposed changes & template

1. +++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleContentTest.php
   @@ -182,25 +182,25 @@ function testContentTypeDirLang() {
   +    $pattern = '|class="[^"]*node--' . $nodes['en']->id() . '[^"]*"[^<>]*lang="en"|';    ¶
   ...
   +    $pattern = '|class="[^"]*node--' . $nodes['en']->id() . '[^"]*"[^<>]*dir="ltr"|';    ¶
   ...
   +    $pattern = '|class="[^"]*node--' . $nodes['es']->id() . '[^"]*"[^<>]*lang="es" dir="ltr"|';    ¶
   

   Whitespace

2. +++ b/core/modules/node/node.module
   @@ -644,10 +644,8 @@ function template_preprocess_node(&$variables) {
   +  $variables['url'] = $node->url();
   ...
   -  $variables['node_url'] = $node->url('canonical', array(
   -    'language' => $node->language(),
   -  ));
   
   +++ b/core/modules/node/templates/node.html.twig
   @@ -20,13 +20,14 @@
   - * - node_url: Direct URL of the current node.
   + * - url: Direct URL of the current node.
   
   @@ -75,27 +76,33 @@
   -      <a href="{{ node_url }}" rel="bookmark">{{ label }}</a>
   +      <a href="{{ node.url }}" rel="bookmark">{{ label }}</a>
   

   This should be {{ url }}, not {{ node.url }}.

3. +++ b/core/modules/node/templates/node.html.twig
   @@ -20,13 +20,14 @@
   + * - author: The node author, using the 'compact' view mode.
   
   +++ b/core/themes/bartik/templates/node.html.twig
   @@ -20,12 +20,12 @@
   + * - author: The node author, using the 'compact' view mode.
   

   I would say instead:

   author: The node author user entity, rendered using the "compact" view mode.

4. +++ b/core/modules/node/templates/node.html.twig
   @@ -20,13 +20,14 @@
   + *   template_preprocess_node().
   

   This doesn't make sense. It's not apart of the previous sentence.

5. +++ b/core/modules/node/templates/node.html.twig
   @@ -75,27 +76,33 @@
   -<article id="node-{{ node.id }}" class="{{ attributes.class }} clearfix"{{ attributes }}>
   +<article class="node--{{ node.id }} {{ attributes.class }}"{{ attributes }}>
   
   +++ b/core/themes/bartik/templates/node.html.twig
   @@ -69,33 +69,36 @@
   -<article id="node-{{ node.id }}" class="{{ attributes.class}} clearfix"{{ attributes }} role="article">
   +<article class="node--{{ node.id }} {{ attributes.class }}"{{ attributes }}>
   

   Just to verify, are we intending to get rid of the clearfix class on both templates? I can see why Stark wouldn't, but shouldn't Bartik?

   Also, semi-related, does this inadvertently leave a trailing space if there are no {{ attributes.class }} present to print?

6. +++ b/core/modules/node/templates/node.html.twig
   @@ -75,27 +76,33 @@
   +    {{ content|without('comment','links') }}
   

   Space after , please.

7. +++ b/core/themes/bartik/templates/node.html.twig
   @@ -69,33 +69,36 @@
   -  {% if content.links %}
   -    <footer class="link-wrapper">
   -      {{ content.links }}
   ...
   +  <div class="link-wrapper">
   +  {{ content.links }}
   +  </div>
   

   The if statement here was lost. Was the change from <footer> to <div> necessary?

Fixed the test & the comments from #92
the test freaked out that i split the comments out of the content & frankly that is another task we should take when we get into comments (yes postponing) for stark

Not all the comments in #92 were addressed:

• #3 - Also Bartik's template. Please keep in mind the 80 character comment limit too (the period goes over)
• #5
• #7

Also, please provide interdiffs so it's easier to see what has changed.

#92.2 - I think part of the point of this issue was to remove the node_url variable and just have Twig call the url method for us by using {{ node.url }}. So I think with the latest patch we can redo that change and remove the 'url' variable from preprocess.

In fact I'd also suggest that 'label' could very likely be {{ node.label }} if we wanted.

#92.5 - This patch should probably make only very minimal changes to Bartik, at least that's what we've been doing for other markup change issues. As for the empty space if attributes.class is empty, some of the approaches taken in #2187113: Incorrect usage of attributes in twig templates resulting in possible duplicate attributes. might be valuable, in this case the |merge filter. In general though I think we need to figure out what to do with attributes in core because adding variables both in preprocess and in the template gets messy pretty quickly.

1. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeFieldMultilingualTestCase.php
   @@ -132,9 +132,9 @@ function testMultilingualDisplaySettings() {
   +      ':node-class' => 'node--' . $node->id(),
   +      ':content-class' => 'node__content',
   

   Need spaces on either side of these variables to make the concat/normalize actually work correctly.

2. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeTitleTest.php
   @@ -60,7 +60,7 @@ function testNodeTitle() {
   +    $this->assertEqual(current($this->xpath('//article[contains(concat(" ", normalize-space(@class), " "), :node-class)]/h2/a/span', array(':node-class' => 'node--' . $node->id() ))), $node->label(), 'Node preview title is equal to node title.', 'Node');
   

   Same here add space on the :node-class value.

3. +++ b/core/modules/node/templates/node.html.twig
   @@ -20,12 +20,12 @@
   + * - author: The node author user entity, rendered using the "compact" view mode.
   

   80 chars

fixing of space 80 char etc - now sleep

Looking good, some minor questions:

1. +++ b/core/modules/node/templates/node.html.twig
   @@ -97,5 +101,4 @@
      </div>
    
      {{ content.links }}
   -
    </article>
   

   This is very minor but I wanted to check it was intentional we're removing the blank line before the closing tag, we have a blank line after the opening tag. Same thing in the Bartik twig file

2. +++ b/core/themes/bartik/templates/node.html.twig
   @@ -20,12 +20,12 @@
   + * - author: The node author, using the 'compact' view mode.
   

   Do we need to specify the view mode in the documentation? Couldn't this easily change and become false? I haven't seen us do this anywhere else. How about just "The author of this node"?

updated as per #102.

1. +++ b/core/modules/node/node.module
   @@ -669,15 +667,15 @@ function template_preprocess_node(&$variables) {
   -      $variables['user_picture'] = user_view($node->getOwner(), 'compact');
   +      $variables['author'] = user_view($node->getOwner(), 'compact');
   
   +++ b/core/modules/node/templates/node.html.twig
   @@ -20,12 +20,12 @@
   - * - user_picture: The node author's picture from user-picture.html.twig.
   + * - author: The node author user entity, rendered using the "compact" view mode
   
   +++ b/core/themes/bartik/templates/node.html.twig
   @@ -20,12 +20,12 @@
   - * - user_picture: The node author's picture from user-picture.html.twig.
   + * - author: The author of this node.
   

   Both Core (Stark) and Bartik templates are now no longer in sync with these documentations.

   It should be clear that this is the node author entity rendered as the "compact" view mode. Yes, it should be explicitly defined here (by default). If a theme overrides a template (and view mode) then it's their job to change the documentation in their overridden template.

   Furthermore, I know it's sad that we're rendering before it even gets to the template, but because we don't have a |view_mode() filter in twig, this is currently unavoidable.

   We must be clear what is being passed to the template, that is the point of documentation: to explicitly define what is being passed to the template; "The author of this node." is simply too ambiguous. Is it the entity, rendered markup, just the name, what?

   Also, when someone raises a question, it's best to discuss it before making the change. Regardless of what the outcome is, please make sure that the 80-character limit is enforced and that it's a complete sentence. The the punctuation doesn't fit, just move the last word to a new line... please, don't remove it.

2. +++ b/core/modules/node/node.module
   @@ -1295,7 +1293,7 @@ function node_form_system_themes_admin_form_submit($form, &$form_state) {
     * implementation may explicitly allow, explicitly deny, or ignore the access
   - * request. If at least one module says to deny the request, it will be rejected.
   + * request. If at least one module says to deny the request, it will be rejected
     * If no modules deny the request and at least one says to allow it, the request
     * will be permitted.
   

   Same here. This should just be restructured so that "rejected." is moved to the next line. The following sentence should also be on that same line and reformatted so it fits within the 80-character limit.

3. +++ b/core/modules/node/templates/node.html.twig
   @@ -75,20 +75,24 @@
   -<article id="node-{{ node.id }}" class="{{ attributes.class }} clearfix"{{ attributes }}>
   +<article class="node--{{ node.id }} {{ attributes.class }}"{{ attributes }}>
   

   Again, I'll ask: are we intentionally removing the "clearfix" class from core's (stark) template?

4. +++ b/core/modules/node/templates/node.html.twig
   @@ -97,5 +101,5 @@
      </div>
    
      {{ content.links }}
   
   +++ b/core/themes/bartik/templates/node.html.twig
   @@ -69,33 +69,36 @@
      <div class="content clearfix"{{ content_attributes }}>
   -    {{ content|without('links') }}
   +    {{ content|without('comment', 'links') }}
      </div>
   ...
   +  {{ content.comment }}
   

   I'm not entirely sure, but I think we lost the core (Stark) template's division of comments from content (#90). @mortendk, was this intentional on your part?

5. +++ b/core/modules/node/templates/node.html.twig
   @@ -97,5 +101,5 @@
   -
   +  ¶
   

   Whitespace.

6. +++ b/core/modules/rdf/rdf.module
   @@ -322,7 +322,8 @@ function rdf_preprocess_node(&$variables) {
   -        // Adds RDFa markup for the comment count near the node title as metadata
   +        // Adds RDFa markup for the comment count near the node title as
   +        // metadata
   

   Missing period.

7. +++ b/core/themes/bartik/templates/node.html.twig
   @@ -69,33 +69,36 @@
   -<article id="node-{{ node.id }}" class="{{ attributes.class}} clearfix"{{ attributes }} role="article">
   +<article class="node--{{ node.id }} {{ attributes.class }}"{{ attributes }}>
   

   Again, I'll ask: are we intentionally removing the "clearfix" class from Bartik's template?

8. +++ b/core/themes/bartik/templates/node.html.twig
   @@ -69,33 +69,36 @@
      {% if content.links %}
   -    <footer class="link-wrapper">
   +    <div class="link-wrapper">
          {{ content.links }}
   -    </footer>
   +    </div>
      {% endif %}
   

   Again, I'll ask: are we intending to change this tag from <footer> to <div> in Bartik's template?

PS: there's already a "Needs issue summary update", but can we please get some clear direction about what this issue is trying to accomplish? There's some scope creep here.

If you don't use Dreditor, you can manually follow the Issue summary instructions

+++ b/core/modules/node/node.module
@@ -644,10 +644,8 @@ function template_preprocess_node(&$variables) {
+  $variables['url'] = $node->url();
 
-  $variables['node_url'] = $node->url('canonical', array(
-    'language' => $node->language(),
-  ));

also, don't remove the args. @see: #2149649-64: Entity rendering/theming does not use the active entity language to render links

@markcarver:
changing the footer to a div for the .link-wrapper, cause its not a footer
clearfix is removed from stark, but will be in bartik, its layout behaviour, so it has nothing to do in stark.
content comments & splitting them in stark is not the role for this patch - i would suggest a follow up patch later if we want stark to do more than it does today

... now trying to do an interdiff... stand clear

interdiff + new patch added

kicks bot

Sorry, I don't get this. The header element for the article is being removed, and part of the header element is being turned into a footer element? It made a lot more sense to me before. The header and footer elements are scoped to their parent element. So it makes perfect sense to have the page title and the submitted info in header, and the links that appear below the content in a footer. I don't think I agree with those changes.

updated the issue summary - hopefully it make more sense now

@ mdrummond
submitted info belongs in the footer tag:

The footer element represents the footer for the section it applies to. A footer typically contains information about its section such as who wrote it, links to related documents, copyright data, and the like.

http://html5doctor.com/the-footer-element/

Having a header around a h2 to make little to no sense, thats just to add in a header tag cause we can ;)

The template looks ok for me so far.

Just a few questions:
* Why do we need the node-id class? Is it nessesary for the contextual edit menu? Other usecases?
* Do we need the div around the real content?

What are the use cases?

We can style alle text content via the body style. All exeptions are in special divs and can be treeted special (author, headline, submittet, links).
The conent consists of field which are wrapped in div with field classes.
This div is one of the first I am removing in every single node template …
I would like to get rid of it forever.

@mdrummond
The use of the footer element is correct – the element (footer) name is misleading.

another cleanup in this patch:
* added is-state classes & corrected all the classnames to follow the code standards
* changed the test for is not page to is teaser, cause it makes more sense to read

fixed the test and went back on the {% if not page %} as it might gonna be out of this scope

my thought were that its not logical what {% if not page %} actually does i would suggest to test against the viewmode instead, as we actually are.

{% if view_mode == teaser %}
  ...

it make immediately sense what it is that the code is doing & shows more possibilitys - maybe that should be in a follow up issue ??

I don't want to derail this with a header and footer argument. But just so people don't think I'm daft, here are the actual definitions of header and footer:

• http://www.w3.org/TR/html5/sections.html#the-header-element
• http://www.w3.org/TR/html5/sections.html#the-footer-element

HTML5 Doctor is a good site with quality advice, but its examples and advice are suggestions, not the spec itself. What the spec says in the footer definition is this:

Contact information for the author or editor of a section belongs in an address element, possibly itself inside a footer. Bylines and other information that could be suitable for both a header or a footer can be placed in either (or neither). The primary purpose of these elements is merely to help the author write self-explanatory markup that is easy to maintain and style; they are not intended to impose specific structures on authors.

I think it's perfectly sensible to consider a heading and byline introductory material that could go into a header. A footer for the submitted info is fine too, but putting links in a footer element makes sense too. So if you want specific grouping elements for those, then putting submitted in a header and links in a footer could work just fine.

However, it's important to note that the heading only shows up here in the teaser view. So in all other situations you'd have the submitted info in a header element all by itself, with no heading. That doesn't really make a lot of sense.

I don't think there's an absolute need to use header and footer elements here either. You could just use divs for the submitted info and for the links, and everything still works just fine. The header and footer are optional elements. I'm not aware of any ways that they actually have an effect on how the page is processed, either by search engines or screen readers. So it might be simpler to just go with divs for the submitted info and the links. A lot less arguing that way!

phew another cleanup:

Lets stop this node--id madness
@derheap #116 .node--$id class you are totally right about the node--$id, i have been theming sites for 8 years now, and i have so far only once used node--$id, and i set that as a class ;) I havent heard about anybody that have ever used the node--id for anything, it smells & looks like legacy + What-if cruft.
If we follow the codestandard & our guidelines this class do not fall into the 90% rule, and its dead easy to put in your self as a themer & why i wanna keep the <articler class="{{ attributes.class }}"{{ attributes }}>so its intuitive for the themer to see where a node--$id can be added.

Unless somebody can raise strong case for keeping .node--$id even as a class -
I suggest that are reaslistic AND remove this abomination of useless information - yes i feel strongly about this... ;)

Wrapper on content
The wrapper is there to have some structure for the inner workings of the node node__content the first time you open up the site + the {{ content_attributes }} is there for modules to ba able to add stuff here, even that i never have seen anything in that placeholder as @scor also comments on in #117

What would be nice was some better documentation for this {{ content_attibutes }} cause it feels & smalls like cruft - but lets do that in a followup issue.

header vs footer ... wtf HTML5
Its seems like we can open up an epic bikeshed over the use of <footer class="node__meta"> vs <header>...<div class="node__meta"> Lets try not to do that ;)
As I see it: Bartik & stark is completely fine with having different markup, Bartik is a theme that have a design on top of it, where stark is just starknaked. In bartik the design have the author info as part of the header (& why mdrummond is *kinda* right ;) Where stark dosnt care and have it seperated out, why that can be a <footer>autor</footer> else i would move that we dont change in the markup as it is now change the classnames to follow the standards & we can do a follow up issue, where the epic battle of author info can start.

this patch:
Removed the node--id competely (gasp!)
Added the preview, sticky, unpublished, preview classes to be modifier classes for node, node--sticky node--unpublished node--preview

Totally for removing node-5 from class and/or id. It just gives false hopes to themers that id is going to be the same between dev/stage/prod environments. Though the id's look unique they are not! The uuid is the unique key but that's nasty and would rather not see that in CSS.

Those special cases where a themer *thinks* they need this, it can be easily added back in. Though it would be recommend to use the node type or a custom class from the UI for that node if necessary... something for contrib to think about.

Thanks for doing this @mortendk!

Very nice Morten, It looks good I did a tiny comment on line 37-38 of node.html.twig. I believe the documentation is wrong since you made the type class node--type-[bundle]. Not sure if I am right, though.

Indeed, looking very good!

Is it within the remit of this issue to clean up the output, especially of user picture / author, or is that in some other issue etc, for example you always get article wrapper even if the user has no picture, why we need that article wrapper I have no idea, I would do something like this:

  $user_picture = user_view($node->getOwner(), 'compact');
  $variables['user_picture'] = isset($user_picture['user_picture']) ? $user_picture['user_picture'] : array();

now you get nothing if there really is no user picture and no article wrapper when there is, just the field wrappers.

Fixed the documentation & the test, added in the {{ attributes|without('class') }} now that the without on attributes came into core.

@jeffburnz lets open up another issue about the user picture and fix that seperate, im pretty sure its gonna open up another can of worms ;)

testbot: get to work

Right you are: #2247677: User picture improvements in node template

@jeff awesome - lets clean that out :)

... sooo can we get an RTBC on the node template ?

This is looking really really good. Just some nit picks and I think I could RTBC this.

1. +++ b/core/modules/node/templates/node.html.twig
   @@ -8,8 +8,8 @@
   + *   - createdtime: Formatted creation date. Preprocess functions can reformat
   

   Can this variable be named created_time so that we are consistent with the naming pattern of _ separating words?

2. +++ b/core/modules/node/templates/node.html.twig
   @@ -33,17 +34,17 @@
     *   - view-mode-[view_mode]: The View Mode of the node; for example, a teaser
     *     would result in: "view-mode-teaser", and full: "view-mode-full".
   

   These lines should be node--view-mode-...

3. +++ b/core/modules/node/templates/node.html.twig
   @@ -75,27 +76,32 @@
   +<article class="{{ attributes.class }}"{{ attributes|without('class') }}>
   

   Since there no clearfix or id, I'd like to see this just {{ attributes }} without|without() so there is no chance of empty class="" attribute if the classes are removed in preprocess and a cleaner template.

4. +++ b/core/modules/node/templates/node.html.twig
   @@ -75,27 +76,32 @@
   +  <div class="node__content"{{ content_attributes }}>
   

   This will duplicate class="", it needs |without('class') or the class needs to be added in preprocess.

5. +++ b/core/themes/bartik/css/style.css
   @@ -669,44 +669,44 @@ h1#page-title {
   -.submitted .field-name-field-user-picture img {
   +.node__meta .field-name-field-user-picture img {
   

   This looks more consistent between the templates, thanks for moving it up.

6. +++ b/core/themes/bartik/templates/node.html.twig
   @@ -33,17 +33,17 @@
     *   - view-mode-[view_mode]: The View Mode of the node; for example, a teaser
     *     would result in: "view-mode-teaser", and full: "view-mode-full".
   

   Same note as before.

7. +++ b/core/themes/bartik/templates/node.html.twig
   @@ -69,33 +69,35 @@
   +  <div class="node__content clearfix"{{ content_attributes }}>
   

   class should be split with |without('class') or you could get duplicates class attributes here. Good idea to dump the classes in after the clearfix too so they don't get lost.

Since there no clearfix or id, I'd like to see this just {{ attributes }} without|without() so there is no chance of empty class="" attribute if the classes are removed in preprocess and a cleaner template.

I understand the motivation here (clean template) but it's also more opaque/abstract for new themers. I tend to think if you're at the level of removing classes in preprocess you know about attributes entirely and understand what's going in the template, in other words the advanced themer does not need hand-holding, but the newbie does a little bit. Front end developers already have the whole power of the attributes system at their fingertips, whereas the new kid on the block just wants to get their class added.

Either way I'm not arguing really strong on this point, but I do tend to agree with this pattern of breaking out the classes in this particular template, mainly because like block its so common to override.

@Jeff Burnz I completely agree and to further that point the first theme a user sees is Bartik, so far we've been putting those examples of how to split attributes out in that theme. We are trying to keep core templates as clean as possible. Does that explain my reasoning better?

I looked through the latest patch to see if I could find any accessibility problems. Looks good!

Only potential problem I can see is the removal of id="node-{{ node.id }}" but that's only a problem if there is something using it. I don't know of anything off hand that does though.

#135 - sure, I'm not strongly for or against either approach.

patch:
* fixed the {{ attributes|without('class') }}
* updated the inline documentation so its now understandable what the variable name is not foo but node.foo.

Theres no reason at all to have concerns about removal of the node-id -its a badpractice + nobody is using it. If a theme wants to use #id they can easy add it in with {{ node.id }}

created_time we should fix that world wide imho - but i would suggest that we take that when we clean up the comments (where it also exist) we get the node patch in, so we can start that work - Cause its been almost a year of nidpicking, so any bikesheeding we can take at the bar (im buying)

@mortendk I remember talking to you about that in Portland, and from a CSS/HTML perspective, absolutely.

However, WAI-ARIA attributes like aria-describedby use ID's as does JS. I don't think that the JS is going to be an issue and can't see where ARIA would at this point either.

I don't think there are any instances where it is needed in this template, I just raised it as something to be considered as there are places where they are necessary for adding semantics.

@mgifford cool so it seems that we agree :)

Yup. Looking forward to seeing this in Core.

/me dances ... and yes im looking forward to get into the comments and clean that up as well :)

yo bot get to work ..

Few more, thanks for the ones before and I agree createtime can change globally elsewhere.

1. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeFieldMultilingualTestCase.php
   @@ -132,9 +132,9 @@ function testMultilingualDisplaySettings() {
   +      ':node-class' => 'node ',
   +      ':content-class' => 'node__content',
   

   'node ' needs a space on both sides to be correct. so ' node '. Unlikely but would also match "article-node ..." or something.

2. +++ b/core/modules/node/lib/Drupal/node/Tests/NodePostSettingsTest.php
   @@ -45,7 +45,7 @@ function testPagePostInfo() {
   +    $elements = $this->xpath('//*[contains(@class,:class)]', array(':class' => 'node__meta'));
   
   @@ -61,7 +61,7 @@ function testPagePostInfo() {
   +    $elements = $this->xpath('//*[contains(@class,:class)]', array(':class' => 'node__meta'));
   

   Space after the @class, commas.

3. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeTitleTest.php
   @@ -60,7 +61,7 @@ function testNodeTitle() {
   +    $this->assertEqual(current($this->xpath('//article[contains(concat(" ", normalize-space(@class), " "), :node-class)]/h2/a/span', array(':node-class' => 'node--type-' . $node->bundle() . ' '))), $node->label(), 'Node preview title is equal to node title.', 'Node');
   

   Add a space before 'node--type-' so ' node--type-'

4. +++ b/core/modules/node/templates/node.html.twig
   @@ -5,14 +5,14 @@
   + *   - node.id: The node ID
   

   Need a period at the end.

5. +++ b/core/modules/node/templates/node.html.twig
   @@ -5,14 +5,14 @@
   + *   - node.bundle: The type of the node, for example, "page" or "article".
   

   I don't know about this node. being prepended because that is not in the coding standards. Maybe Cottser can confirm if this is right or not?

6. +++ b/core/modules/node/templates/node.html.twig
   @@ -5,14 +5,14 @@
   + *   - node.createdtime: Formatted creation date. Preprocess functions can reformat
   

   80 chars.

7. +++ b/core/modules/node/templates/node.html.twig
   @@ -75,27 +77,31 @@
   +<article {{ attributes }}>
   

   Remove the space before the attributes here.

heres more for you joel

Getting there, can we revert the node. additions to the docblocks so we are consistent with https://drupal.org/node/1823416 ?

1. +++ b/core/modules/node/templates/node.html.twig
   @@ -5,11 +5,11 @@
     *   - node.bundle: The type of the node, for example, "page" or "article".
     *   - node.authorid: The user ID of the node author.
   ...
     *   - node.promoted: Whether the node is promoted to the front page.
     *   - node.sticky: Whether the node is 'sticky'. Sticky nodes are ordered above
   

   Though I agree this is clearer maybe we can open up a twig docblock issue to discuss how to be consistent and clear as we discussed in the hangout?

2. +++ b/core/modules/node/templates/node.html.twig
   @@ -37,15 +37,16 @@
   + *   - node---sticky: Appears on nodes ordered above other non-sticky nodes in
   + *     teaser listings.
   + *   - node---unpublished: Appears on unpublished nodes visible only to site
   + *     admins.
   

   Just realized these have a few too many ---s

3. +++ b/core/modules/node/templates/node.html.twig
   @@ -94,7 +95,7 @@
   +  <div class="node__content{{ content_attributes.class }}"{{ content_attributes|without('class') }}>
   

   Whoops this one needs the space before the class. Attribute renders with a space before because all attributes have that, but the values don't prepend a space.

fixed the documentation things & changed the test so it works with .node__content

Die ID, Die!!

Found some more missing bits, please review the interdiff to make sure these make sense.

@joelpittet sweet yup it looks good.

+++ b/core/modules/node/node.module
@@ -586,7 +586,7 @@ function node_is_page(NodeInterface $node) {
+    $variables['attributes']['class'][] = drupal_html_class('node--type-' . $node->getType());

ooh good catch - yes its a modifier for node

i dont think i can put this to be RTBC, cause of looong work on this patch, its itching in my fingers though.
but to me it looks ready to roll (i sure hope)

Needs before and after screenshots/manual testing of node pages and teasers in stark, bartik and seven.

Heres the 12 screenshots that shows that the patch is not changing the visuals - to not overload the page, im not embedding them on the page - this page is long enough all ready.

Did 3 things.
Fixed the margin around 'submitted' element shown in the screenshots above. There is no P tag so it will now have no margin on stark, but I've re-applied it on bartik doesn't care because it removed that p tag.

I moved the RDF stuff to the template for both the author_attributes and metadata. Need an RDF person to take a look to see if this is cool or a better way to do this?

And used the trans tag to get submitted string into the template.

This should fix those exceptions and errors for rdf in the last patch.

+++ b/core/themes/bartik/templates/node.html.twig
@@ -69,33 +69,38 @@
+        {{ author_picture }}

the data comes from the user and can in principle be whatever the editor have added into the compact view mode right? - so to be nidpicking, is author_picture the right name or should we instead grap that user image directly - and not use the entity ? that would make more sense, but maybe that should be a seperate issue...

if we choose to create another issue, then its screenshot time again ;)

About the author_picture:

but maybe that should be a seperate issue...

Yes, another issue please. Is #2247677 the right place for that?

1. +++ b/core/modules/node/templates/node.html.twig
   @@ -75,27 +76,34 @@
   +      {{ author }}
   

   should be {{ author_picture }}

2. +++ b/core/modules/node/templates/node.html.twig
   @@ -75,27 +76,34 @@
   +        {{ metadata }}
   

   Will wait for scor to discuss, however for now its there but no comments in the docblock about what it is or what it does etc.

I am sure you guys have some fang dangled argument about this, but honestly I do not get this idea of no classes on titles in block and node - you force element selector usage and deep nesting of selectors, even for silly simple things like wanting the node title and node content h2 to have a different font. h2 is an element, so give it the component level selector node__title, without I have am forced to:

.node h2 {}
.node__content h2 {}

forgive me, that that just ain't BEM style, so no, We aint rockin' it (not contagious), dis jus monkey business (outrageous...)... :/

I'd agree that there should probably be a class on those h2 elements. Maybe that's hidden in title_attributes, but if not, would be good to get that in there. Probably node__title. Might want node__link on the anchor inside too.

Seriously guys...

Forgive me but can we put your concerns into a follow up issue and lets get this patch in ?

its extremely frustrating that we have used almost a year to get this in - Using almost 1 year on cleaning up the node.html.twig file is absurd - seriously it should be done within 2-3 weeks

I personally have strong objections about having a preprocess that creates hardcoded classes (and using attributes to put that in), but those are gonna be in a followup issue - why, Cause we need a cleaner node to get in to be able to move forward.

At this speed were barely walking.

I can revert and split the submitted move to trans block in the template to a new issue if that helps any?

sorry if i seem to rant here but (thats the nice way of saying rant alert)

It seems like the discussions that we have had the last 2 years dont matters anymore & were desperately trying to put on classes on everything we can find in the case of: ooh no the themer have to add in the classes s/he needs.

Seriously wasn't there an agreement of "start with nothing & then only add on as needed" ? - Did we forget that or do we have to remind ourself of that every 2 weeks ?
-> https://drupal.org/node/2008464#principles

We can put whatever we want on Bartik as we sees it (it can drown for all that matters in divs & classes)
But wasnt the idea behind stark to keep it as clean as possible - this is ment as the minimum base. NOT the base thats everything you can possible think about. Else what were gonna end up with is just another 4.5,6,7 Drupal version with a {{ wooho }}

Else we need to discuss the idea of having a "Drupal Base" as a basetheme, then we can add in all the things that are atm belived to be the best practive (and that we know will be out of date in 12 months)

Sorry guys but we need to keep focus on the bigger picture here - and i feel that we are not.

Morten - I am not raising some discussion about the principles, or that this should be an attributes class (it should not IMO), however those principles do not discount the raising of this specific issue - there is nothing in those principals about adding specific classes to one element, but not others - they're a general guide, which is subject to interpretation.

For example: Consistency: Do the same things everywhere, follow patterns. Not having an h2 class is incosistant with other elements in the template, here's the most obvious example: <footer class="node__meta"> - here we have a semantic sectioning element that does not actually require this class to be styled via CSS, because you can do: .node footer {}.

The "consistent" pattern being established here is BEM, but we're not being consistent with it. If we look at the template as a whole, we have a BEM class on practically all elements, except one - and one that is highly likely to be repeated in the actual node content (h2). By not having that class we loose the advantage of BEM - flat specificity and maintainability.

What is the point of adding these BEM classes in the first place if those are not the goals?

Why add node__content, node__meta, node__links etc, none of which are actually required other than to provide convenient hooks for themers?

We can consider: Build from use cases: Think about the 90% of use cases - It would be my interpretation that 90% of the time users would want to specifically style titles and headings in nodes, I'm not saying the can't, but they would be forced to override the template or use preprocess if they want to conform (be consistent) with BEM, or rely on HTML structure and then having to deal with specificity issues - again that feels very inconsistent with the BEM concepts and patterns being established in our core templates.

These are fair questions that deserve a fair and reasonable technical discussion, I would kindly ask you to not rant at me every time I raise such issues, even if this takes more time.

[edit, added a code tag]

@ Jeff Burnz
@mdrummond

Let us please remove stuff – according to the agreed guidelines – and get this RTBC.

Stark should be as clean as possible – the ground to build on. Not the ground to start removing stuff.
If you need an node_title for your styling – fine, add it to your template.

I dont see a usecase für .node h2 {}. As nodes are the content of the site, most h2 will be node titles. So they are the rule, not the exeption.

And .node__content h2 {} should not be nesseccary, as there should be no h2 inside node_content.

And even the classitis rich Bartik does not put a class on the h2 of a node.
So don’t let us do worse than Bartik.

For me this issue is not going far enough. I would like to remove
<div class="node__content"{{ content_attributes }}> too.

But that might be another issue.

So lets close this can of worms and get it in core.

For everything else please open another issue.

#167, generally I agree with this, but those hooks are pretty handy for 90% of use cases so I'm not sure we need to throw baby out with bathwater. Top my mind they mostly meet the principle of 90% use case - e.g. node__meta lets you drill down to field type image and img without too many problems later on, etc etc (like not bleeding styles into user template).

"there should be no h2 inside node_content" is not quite right, in html5 you can start any new section with whatever heading level you want. The point of BEM is to be able to largely ignore such structural concerns.

It's not a can of worms, it's not that serious imo, its a simple yes/no should we agree to be inconsistent or not, you guys are making this a big deal, I simply asked WHY it is like this, and so far Morten sites some principles and ranted on something about Bartik and you want to rip everything out, sorry but I am not quite sure why either of you would have such strong reactions to someone simply pointing out a very obvious inconsistency.

What im "ranting" about is our process that is completely broken & constantly stop patches - and that is draining energy. This patch is almost a year old for crying out loud. When we constantly stop on small details - i really dont care that much about node__title or whatever, then were stopping ourself to move forward.

So to take it on a technical level using selectors is not against our css manual
Using a h2 for a title and remove the old .title class makes sense cause it makes the template cleaner and its really easy to target with a .node > h2 not to mention that this h2 is only a thing we have in the teasers view (out of the box anyways)
Its been raised many many times by many a themer "why dont we just use the h2 tag" - that imho makes sense, yes it also make sense to have a node__title (same thing as footer vs div etc) but should we bikesheed it for weeks, and stop a patch from getting in.

as a sidenode we are building on smacss with some inspiration from bem

If you guys really really really think that not having a .node__title is a blocker for this patch (really ?) Then create the patch so we can move forward.

Lets not make perfect the enemy of good.

The problem stated for this issue is that we want to cleanup node.html.twig so that it follows CSS coding standards.

Good goal.

Here is our CSS architecture guidelines: https://drupal.org/node/1887918

Best practice one is to avoid reliance on HTML structure in CSS rules. We can only do that if we have the classes necessary in HTML to allow us to do that.

I understand your frustration, and where you're coming from Morten. I am just as on board as you with ripping out divs and classes that are unnecessary.

Frankly the object oriented CSS principles we're using for Drupal 8 are new to me. I've typically based my CSS on element selectors. I'm trying to learn this new way of doing things, and as I'm learning about it, it does make sense.

We can certainly create a follow-up issue to deal specifically with the node teaser title. (Now I'm thinking we may want a class of node--teaser__title and possibly node--teaser__title__link to more accurately reflect when this would appear.) However I do think that figuring this out is in scope for this issue, which is getting the markup for the node template to match up with our CSS guidelines and thus our CSS architecture.

My understanding is that there has been an agreement to use these SMACSS/OOCSS/BEM principles in Drupal 8's markup and CSS. If so, then we should be putting that into place in our markup.

...so as a followup question here over my morning coffee & cursing over the issueque and reading up on jeffburnz points on not beeing consistent :)

stark naked ?
We agreed to keep stark clean right ? now we have added in node__footer, node__meta etc to core, dammit thats against the principle so that should be ripped out and - are we are failing our own mission?

bartik
Good old bartik should have all those classes .node__title .node__footer .node__whatever goes in there.

A thing we know is that theres a huge wish for simple classes and a simple consitency in out of the box of drupal, even that some of us want a complete naked theme to work out from. that means a few basic classes, that is simple to remove (so they are in the tempate file).

To move forward. I would suggest that we add in the .node__title to bartik, keep that out of stark (as jeff points correctly out) and get this baby in.

May i suggest that we then create a followup issue that can take on the discussion about how little or much stark is coming with, the role of bartik, the role of a drupalbase theme etc. ?

rolled a patch with the node__title added to bartik not to stark, adde the {{ author_picture }} as well + a really lame documentation ;) for {{ metadata }} which im not sure why we need that i the first place @joelpittet ?

I think the issue is that this isn't consistent.

System/stark right now has the following BEM-style classes: node__meta, node__submitted, node__content, node__links.

The article element also has some node classes on it in system/Stark.

The only element in here that doesn't have a BEM-style class name in system/Stark is the node teaser heading and the link that appears inside it.

I don't know why it's ok for all the other elements to have those BEM-style classes, but how doing the same thing for this one element is the end of the world as far as having semantic markup.

If the goal is to strip every single class out of system/Stark markup, that's one thing. That's not my understanding of what we're trying to do with Drupal's default markup, and I'd imagine there are probably a number of others out there who also don't agree with that.

But if we're going to try to commit this patch that has BEM-style classes for most of the elements in the markup for Stark, it would seem to make sense to do that for the node teaser heading as well.

@mdrummond: As i see it about stark: We are trying to create it with small impact on the markup as possible. Just because we are using a bem style class naming on a couple of elements is not the same as stark have to add a class to everything, as the case is here with the h2

Theres no way to target the links unless there's a .node__links (it used to be .links) .node__meta make sense in the same way. node__content as well make "sense" the H2 dosn't need to have that class, thats why we agreed afaik here in this issue back in july 2013 (comment #31+)
(how many times did anybody use a h2.title ?)

If anybody sees the patch at this state to need more work then put the status to "Needs work" ! Else its green & been hammered on for almost 1 year.

We can alternative:
1. Add a patch that change whatever it is that "needs work" and is blocking the node to be cleaned up.
2. Accept the state of the current node in core and drop fixing it cause of nipicking.
3. Use another 3 months on this, just to see if we can get it "perfect".
4. Bikeshed over whatever we can find (and i can find a few things as well)
5. Add follow up issues to clean up whatever follow ups that might happen

Seriously we cant keep doing this "Best practice of the issuque" where its whomever is willing to fight the longest wins, its retarded and just drains us all out.

So what is it gonna be ?
- yes im totally pulling a trigger here, sorry for that (not sorry) ;)

followup issues:
* A cleaner stark (remove all node--foo classes etc)
* picture article fun as jeffburnz created.
* node-classes out of the preprocess into the template.
* an epic bartik 2.0

(edited: added in followup issue suggestions)

I am not completely happy with the result, but I am fine with it.

As I have found not nitpicks in the interdiff I put it to RTBC.
All other stuff should be followup issues.

Let's take care of the rest in followup issues. Incremental progress is a good thing.

followup issues:
* A cleaner stark (remove all node--foo classes etc)
* picture article fun as jeffburnz created.
* node-classes out of the preprocess into the template.

Looks good.

Here is all the screenshots.
bartik + seven is completely identical as they should be.
Stark had a padding that was added to the submitted, cause it was wrongly wrapped with a <p> instead of a div - this have been changed to a div. Stark is not a design - so it gives no meaning to add in the <p> margin that a browser would add to the tag ( margin: 1em before & after).

Enjoy