Problem/Motivation

node.html.twig markup needs a cleanup after the move from phptemplate to twig.
Both Stark & Bartik needs this, but is not expected to have the same output.

Issues have been raised doing the cleanup those are created as followup issues to seperate the issues & beeing able to move forward

Proposed resolution

basic stark template:

<article class="{{ attributes.class }}"{{ attributes|without('class') }}>

  {{ title_prefix }}
  {% if not page %}
    <h2{{ title_attributes }}>
      <a href="{{ url }}" rel="bookmark">{{ label }}</a>
    </h2>
  {% endif %}
  {{ title_suffix }}

  {% if display_submitted %}
    <footer>
    <footer class="node__meta">
      {{ author }}
      {{ submitted }}
    </footer>
  {% endif %}

  <div class="node__content"{{ content_attributes }}>
    {{ content|without('links') }}
  </div>

  {% if content.links %}
    <div class="node__links">
      {{ content.links }}
    </div>
  {% endif %}

</article>

attributes.class:
.node
.node--$nodetype
.node--promoted
.node--sticky
.node--unpublished
.node--preview

Remaining tasks

* provide screenshots
* inline documentation
* stop score creeping

User interface changes

API changes

none

Please note, we need to keep the variables in sync with the comment template:
#2031883: Markup for: comment.html.twig, issue for global changes #2004966: Markup and variable cleanup for titles and attributes in all templates
Part of: #1980004: [meta] Creating Dream Markup

original proposal

During the code sprint at DrupalCon Portland, Jen Lampton proposed an improved version for node.html.twig:

Proposal

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

  {{ title.prefix }}
  {% if not page %}
    <h2{{ title.attributes }}><a href="{{ url }}" rel="bookmark">{{ title.label }}</a></h2>
  {% endif %}
  {{ title.suffix }}

  {% if display_submitted %}
    <footer>
      {{ author|view_mode('bio') }}
      {% trans %}
        Submitted by {{ author.name }} on {{ published_date|format_date('medium') }}
      {% endtrans %}
    </footer>
  {% endif %}

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

  {{ links }}

  {{ comments }}

</article>

Please note, we need to keep the variables in sync with the comment template:
#2031883: Markup for: comment.html.twig, issue for global changes #2004966: Markup and variable cleanup for titles and attributes in all templates

Part of:
#1980004: [meta] Creating Dream Markup

Files: 
CommentFileSizeAuthor
#205 2004252-node-205.patch27.38 KBsarahjean
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,789 pass(es). View
#190 Screen Shot 2014-05-07 at 00.06.42.png475.22 KBJeff Burnz
#181 stark-node-pre.png1.26 MBmortendk
#181 stark-node-post.png1.26 MBmortendk
#181 stark-frontpage-pre.png1.05 MBmortendk
#181 stark-frontpage-post.png1.04 MBmortendk
#181 seven-node-pre.png1.19 MBmortendk
#181 seven-node-post.png1.19 MBmortendk
#181 seven-frontpage-pre.png966.61 KBmortendk
#181 seven-frontpage-post.png966.61 KBmortendk
#181 bartik-node-pre.png1.34 MBmortendk
#181 bartik-node-post.png1.34 MBmortendk
#181 bartik-frontpage-pre.png1.1 MBmortendk
#181 bartik-frontpage-post.png1.1 MBmortendk
#172 2004252-node-171.patch27.5 KBmortendk
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2004252-node-171.patch. Unable to apply patch. See the log in the details link for more information. View
#172 interdiff-node-157-171.txt2.66 KBmortendk
#157 interdiff.txt2.76 KBjoelpittet
#157 2004252-twig-node-cleanup-157.patch27.27 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,626 pass(es). View
#155 interdiff.txt8.16 KBjoelpittet
#155 2004252-twig-node-cleanup-155.patch27.07 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,680 pass(es), 5 fail(s), and 20 exception(s). View
#154 node-bartik-frontpage-before.png977.33 KBmortendk
#154 node-bartik-frontpage-post.png958.53 KBmortendk
#154 node-bartik-before.png1.38 MBmortendk
#154 node-bartik-post.png1.36 MBmortendk
#154 node-seven-frontpage-before.png887.98 KBmortendk
#154 node-seven-frontpage-post.png878.3 KBmortendk
#154 node-seven-before.png1.28 MBmortendk
#154 node-seven-post.png1.27 MBmortendk
#154 node-stark-frontpage-before.png1005.79 KBmortendk
#154 node-stark-frontpage-post.png980.57 KBmortendk
#154 node-stark-before.png1.38 MBmortendk
#154 node-stark-post.png1.36 MBmortendk
#150 interdiff.txt3.62 KBjoelpittet
#150 2004252-twig-node-cleanup-150.patch24.54 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,709 pass(es). View
#148 interdiff-node-145-148.txt4.54 KBmortendk
#148 node-2004252-148.patch22.18 KBmortendk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,692 pass(es). View
#145 node-2004252-145.patch23.08 KBmortendk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,574 pass(es). View
#145 interdiff-node-136-145.txt7.58 KBmortendk
#138 interdiff-node-129-136.txt6.4 KBmortendk
#138 node-2004252-136.patch22.96 KBmortendk
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,573 pass(es), 1 fail(s), and 0 exception(s). View
#129 node-2004252-129.patch20.87 KBmortendk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,310 pass(es). View
#124 interdiff-node-120-124.txt7.37 KBmortendk
#124 node-2004252-124.patch20.26 KBmortendk
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,543 pass(es), 1 fail(s), and 0 exception(s). View
#120 node-2004252-120.patch20.04 KBmortendk
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,581 pass(es), 2 fail(s), and 0 exception(s). View
#120 interfiff-node-2004252-113-117.txt2.86 KBmortendk
#117 interfiff-node-2004252-109-113.txt9.05 KBmortendk
#117 node-2004252-117.patch18.88 KBmortendk
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,497 pass(es), 3 fail(s), and 0 exception(s). View
#110 node-2004252-109.patch14.26 KBmortendk
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,562 pass(es), 1 fail(s), and 0 exception(s). View
#110 interfiff-node-2004252-103-109.txt2.68 KBmortendk
#103 node-2004252-103.patch14.35 KBvisabhishek
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,456 pass(es). View
#103 interdiff-2004252-100-103.txt905 bytesvisabhishek
#100 node-2004252-100.patch14.37 KBmortendk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,040 pass(es). View
#96 node-2004252-96.patch11.79 KBmortendk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,108 pass(es). View
#90 node-2004252-90.patch11.03 KBmortendk
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,013 pass(es), 2 fail(s), and 0 exception(s). View
#89 node-stark-teaser-after.png.png104.2 KBmortendk
#89 node-stark-teaser-before.png104.99 KBmortendk
#89 node-stark-before.png115.43 KBmortendk
#89 node-stark-after.png111.01 KBmortendk
#89 node-bartik-teaser-before.png87.57 KBmortendk
#89 node-bartik-teaser-after.png91.03 KBmortendk
#89 node-bartik-before.png107.94 KBmortendk
#89 node-bartik-after.png114.29 KBmortendk
#88 node-2004252-85.patch11.05 KBmortendk
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,085 pass(es), 2 fail(s), and 0 exception(s). View
#86 node-html-after.png54.49 KBrteijeiro
#85 node-2004252-85.patch10.92 KBvisabhishek
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,031 pass(es). View
#85 interdiff-2004252-83-85.txt2.86 KBvisabhishek
#83 node-2004252-83.patch10.83 KBmortendk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,976 pass(es). View
#81 node-2004252-81.patch10.66 KBManuel Garcia
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,073 pass(es), 4 fail(s), and 0 exception(s). View
#79 node-2004252-79.patch10.66 KBmortendk
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,978 pass(es), 4 fail(s), and 0 exception(s). View
#72 node-2004252-72.patch7.63 KBmortendk
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,015 pass(es), 4 fail(s), and 0 exception(s). View
#69 node-2004252-69.patch7.37 KBcwells
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,039 pass(es). View
#67 node-2004252-67.patch7.27 KBcwells
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,011 pass(es). View
#62 drupal-node-twig-markup-2004252-62.patch6.92 KBnikhiltri
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,954 pass(es), 29 fail(s), and 0 exception(s). View
#60 interdiff-2004252-58-60.txt335 bytesacouch
#60 node-2004252-60.patch6.93 KBacouch
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,874 pass(es), 31 fail(s), and 0 exception(s). View
#58 node-2004252-58.patch6.92 KBDragan Eror
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch node-2004252-58.patch. Unable to apply patch. See the log in the details link for more information. View
#45 node-2004252-45.patch6.78 KBbabruix
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,692 pass(es), 34 fail(s), and 0 exception(s). View
#45 interdiff-2004252-41-45.txt1.13 KBbabruix
#41 node-2004252-41.patch6.78 KBbabruix
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,300 pass(es), 34 fail(s), and 0 exception(s). View
#41 interdiff.txt2.01 KBbabruix
#37 node-2004252-37.patch6.78 KBbabruix
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,368 pass(es), 36 fail(s), and 3 exception(s). View
#34 node-2004252-34.patch6.65 KBbabruix
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,285 pass(es), 173 fail(s), and 4 exception(s). View
#25 node-2004252-24.patch10.88 KBminneapolisdan
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
#18 node-2004252.diff10.92 KBmortendk
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/node/node.module. View
#13 core-markup-node-2004252-12.patch11.3 KBjenlampton
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-markup-node-2004252-12.patch. Unable to apply patch. See the log in the details link for more information. View
#13 interdiff.txt8.31 KBjenlampton
#8 core-markup-node-2004252-8.patch4 KBdasjo
FAILED: [[SimpleTest]]: [MySQL] 56,491 pass(es), 20 fail(s), and 987 exception(s). View
#8 core-markup-node-2004252-8-interdiff.txt3.31 KBdasjo
#5 core-markup-node-2004252-5.patch1.27 KBdasjo
FAILED: [[SimpleTest]]: [MySQL] 56,669 pass(es), 4 fail(s), and 0 exception(s). View
#5 core-markup-node-2004252-5-interdiff.txt537 bytesdasjo
#4 core-markup-node-2004252-4.patch1.02 KBdasjo
FAILED: [[SimpleTest]]: [MySQL] 55,821 pass(es), 6 fail(s), and 0 exception(s). View
#4 core-markup-node-2004252-4-bartik-diff.txt1.52 KBdasjo

Comments

scor’s picture

Component: node system » markup
Issue tags: +needs accessibility review, +Twig, +theme system cleanup, +dreammarkup

tagging

pixelwhip’s picture

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.

ry5n’s picture

#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}}?

dasjo’s picture

FileSize
1.52 KB
1.02 KB
FAILED: [[SimpleTest]]: [MySQL] 55,821 pass(es), 6 fail(s), and 0 exception(s). View

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.

dasjo’s picture

Status: Active » Needs review
FileSize
537 bytes
1.27 KB
FAILED: [[SimpleTest]]: [MySQL] 56,669 pass(es), 4 fail(s), and 0 exception(s). View

removed the comment related to the id attribute

Status: Needs review » Needs work

The last submitted patch, core-markup-node-2004252-5.patch, failed testing.

jenlampton’s picture

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.
dasjo’s picture

Status: Needs work » Needs review
FileSize
3.31 KB
4 KB
FAILED: [[SimpleTest]]: [MySQL] 56,491 pass(es), 20 fail(s), and 987 exception(s). View

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?

Status: Needs review » Needs work

The last submitted patch, core-markup-node-2004252-8.patch, failed testing.

ry5n’s picture

Status: Needs work » Needs review

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

jenlampton’s picture

Status: Needs review » Needs work

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

ry5n’s picture

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

jenlampton’s picture

FileSize
8.31 KB
11.3 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-markup-node-2004252-12.patch. Unable to apply patch. See the log in the details link for more information. View

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

ry5n’s picture

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

ry5n’s picture

Issue summary: View changes

remainder change

jenlampton’s picture

Issue summary: View changes

updated as per recent comment

jenlampton’s picture

@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

jenlampton’s picture

Issue summary: View changes

add link to comment

jenlampton’s picture

Issue summary: View changes

blocked

steveoliver’s picture

Status: Needs work » Needs review

Bot?

Status: Needs review » Needs work

The last submitted patch, core-markup-node-2004252-12.patch, failed testing.

mortendk’s picture

FileSize
10.92 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/node/node.module. View
mgifford’s picture

I just looked over the code...

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

Cottser’s picture

Status: Needs work » Needs review

Testbot?

Status: Needs review » Needs work

The last submitted patch, node-2004252.diff, failed testing.

Fabianx’s picture

Issue tags: +Novice

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

Fabianx’s picture

Issue summary: View changes

remove comment, add view mode

markcarver’s picture

stevector’s picture

minneapolisdan’s picture

FileSize
10.88 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

Rerolling patch from #18 and fixing syntax error.

Les Lim’s picture

Status: Needs work » Needs review

changing status.

Les Lim’s picture

Issue summary: View changes

Updated issue summary.

Fabianx’s picture

Testbot, get to work!

markcarver’s picture

Status: Needs review » Needs 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/>.

markcarver’s picture

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

mortendk’s picture

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

mortendk’s picture

Issue summary: View changes

Updated issue summary.

Jeff Burnz’s picture

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.

mortendk’s picture

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.

Jeff Burnz’s picture

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.

Jeff Burnz’s picture

Issue summary: View changes

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

dead_arm’s picture

Issue summary: View changes

Add related follow up

babruix’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
6.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,285 pass(es), 173 fail(s), and 4 exception(s). View

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

joelpittet’s picture

Status: Needs review » Needs work
  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.

The last submitted patch, 34: node-2004252-34.patch, failed testing.

babruix’s picture

Status: Needs work » Needs review
FileSize
6.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,368 pass(es), 36 fail(s), and 3 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 37: node-2004252-37.patch, failed testing.

droplet’s picture

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

joelpittet’s picture

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.

babruix’s picture

Status: Needs work » Needs review
FileSize
2.01 KB
6.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,300 pass(es), 34 fail(s), and 0 exception(s). View

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

joelpittet’s picture

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?

Status: Needs review » Needs work

The last submitted patch, 41: node-2004252-41.patch, failed testing.

Cottser’s picture

Category: Feature request » Task

Not really a feature IMO :)

babruix’s picture

Status: Needs work » Needs review
FileSize
1.13 KB
6.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,692 pass(es), 34 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 45: node-2004252-45.patch, failed testing.

Cottser’s picture

Issue tags: +sprint

Tagging for focus.

mortendk’s picture

Dragan Eror’s picture

Assigned: Unassigned » Dragan Eror

Will check this one...

Dragan Eror’s picture

Assigned: Dragan Eror » Unassigned
Status: Needs work » Needs review

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

Dragan Eror’s picture

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

dasjo’s picture

Status: Needs review » Needs work
+++ 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

Dragan Eror’s picture

Assigned: Unassigned » Dragan Eror
Issue tags: +drupaldevdays

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

The last submitted patch, 45: node-2004252-45.patch, failed testing.

mortendk’s picture

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

LewisNyman’s picture

  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

dasjo’s picture

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

Dragan Eror’s picture

FileSize
6.92 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch node-2004252-58.patch. Unable to apply patch. See the log in the details link for more information. View

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

Dragan Eror’s picture

Assigned: Dragan Eror » Unassigned
acouch’s picture

Status: Needs work » Needs review
FileSize
6.93 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,874 pass(es), 31 fail(s), and 0 exception(s). View
335 bytes

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.

Status: Needs review » Needs work

The last submitted patch, 60: node-2004252-60.patch, failed testing.

nikhiltri’s picture

FileSize
6.92 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,954 pass(es), 29 fail(s), and 0 exception(s). View

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

visabhishek’s picture

Status: Needs work » Needs review

Changing Status to review.....

The last submitted patch, 58: node-2004252-58.patch, failed testing.

scor’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/node/templates/node.html.twig
    @@ -20,14 +20,13 @@
      * - display_submitted: Whether submission information should be displayed.
    - * - submitted: Submission information created from name and date during
      *   template_preprocess_node().
    

    we're not removing the submitted variable in this issue, in fact it is used further down in the template.

  2. +++ b/core/modules/rdf/rdf.module
    @@ -278,7 +278,7 @@ function rdf_preprocess_node(&$variables) {
       $bundle_mapping = $mapping->getPreparedBundleMapping('node', $bundle);
    -  $variables['attributes']['about'] = empty($variables['node_url']) ? NULL: $variables['node_url'];
    +  $variables['attributes']['about'] = empty($variables['url']) ? NULL: $variables['url'];
       $variables['attributes']['typeof'] = empty($bundle_mapping['types']) ? NULL : $bundle_mapping['types'];
    

    we're missing the corresponding variable name change in template_preprocess_node(), which is why the RDF tests are failing.

The last submitted patch, 62: drupal-node-twig-markup-2004252-62.patch, failed testing.

cwells’s picture

Status: Needs work » Needs review
FileSize
7.27 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,011 pass(es). View

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.

scor’s picture

Status: Needs review » Needs work
+++ b/core/modules/node/node.module
@@ -648,6 +648,9 @@ function template_preprocess_node(&$variables) {
   $variables['node_url'] = $node->url('canonical', array(
     'language' => $node->language(),
   ));
+
+  $variables['url'] = $node->url();

I believe we want to get rid of node_url and use url instead.

cwells’s picture

Status: Needs work » Needs review
FileSize
7.37 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,039 pass(es). View
  • get rid of node_url
  • get rid of role="article" in core node template
  • get rid of role="article" in Bartik node template
mortendk’s picture

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

rteijeiro’s picture

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

mortendk’s picture

FileSize
7.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,015 pass(es), 4 fail(s), and 0 exception(s). View

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

Dragan Eror’s picture

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?

Status: Needs review » Needs work

The last submitted patch, 72: node-2004252-72.patch, failed testing.

mortendk’s picture

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

cwells’s picture

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

Dragan Eror’s picture

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

mortendk’s picture

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

mortendk’s picture

Status: Needs work » Needs review
FileSize
10.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,978 pass(es), 4 fail(s), and 0 exception(s). View

id's changed to classes for testbot ...

Status: Needs review » Needs work

The last submitted patch, 79: node-2004252-79.patch, failed testing.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
10.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,073 pass(es), 4 fail(s), and 0 exception(s). View

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?

Status: Needs review » Needs work

The last submitted patch, 81: node-2004252-81.patch, failed testing.

mortendk’s picture

Status: Needs work » Needs review
FileSize
10.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,976 pass(es). View

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

jibran’s picture

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

visabhishek’s picture

FileSize
2.86 KB
10.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,031 pass(es). View

patch updated as per #84.

rteijeiro’s picture

Status: Needs review » Needs work
FileSize
54.49 KB

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

Before

Only local images are allowed.

After

mortendk’s picture

Assigned: Unassigned » mortendk

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

mortendk’s picture

Status: Needs work » Needs review
FileSize
11.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,085 pass(es), 2 fail(s), and 0 exception(s). View

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

mortendk’s picture

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

mortendk’s picture

FileSize
11.03 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,013 pass(es), 2 fail(s), and 0 exception(s). View

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

mortendk’s picture

Issue summary: View changes

updated the issue with proposed changes & template

markcarver’s picture

Status: Needs review » Needs work
  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?

The last submitted patch, 88: node-2004252-85.patch, failed testing.

markcarver’s picture

The last submitted patch, 90: node-2004252-90.patch, failed testing.

mortendk’s picture

Status: Needs work » Needs review
FileSize
11.79 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,108 pass(es). View

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

markcarver’s picture

Status: Needs review » Needs work

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.

Cottser’s picture

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

joelpittet’s picture

  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

mortendk’s picture

Status: Needs work » Needs review
FileSize
14.37 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,040 pass(es). View

fixing of space 80 char etc - now sleep

mortendk’s picture

Issue summary: View changes
LewisNyman’s picture

Issue summary: View changes

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

visabhishek’s picture

FileSize
905 bytes
14.35 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,456 pass(es). View

updated as per #102.

markcarver’s picture

Status: Needs review » Needs work
  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?

markcarver’s picture

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

droplet’s picture

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

mortendk’s picture

Issue summary: View changes
mortendk’s picture

Issue summary: View changes
mortendk’s picture

@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

mortendk’s picture

FileSize
2.68 KB
14.26 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,562 pass(es), 1 fail(s), and 0 exception(s). View

interdiff + new patch added

mortendk’s picture

Status: Needs work » Needs review

kicks bot

mdrummond’s picture

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.

mortendk’s picture

Issue summary: View changes

updated the issue summary - hopefully it make more sense now

mortendk’s picture

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

Status: Needs review » Needs work

The last submitted patch, 110: node-2004252-109.patch, failed testing.

derheap’s picture

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?

{{ content|without('links') }}

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.

mortendk’s picture

FileSize
18.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,497 pass(es), 3 fail(s), and 0 exception(s). View
9.05 KB

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

mortendk’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 117: node-2004252-117.patch, failed testing.

mortendk’s picture

Status: Needs work » Needs review
FileSize
2.86 KB
20.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,581 pass(es), 2 fail(s), and 0 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 120: node-2004252-120.patch, failed testing.

mdrummond’s picture

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:

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!

scor’s picture

  1. +++ b/core/modules/node/node.module
    @@ -685,23 +685,23 @@ function template_preprocess_node(&$variables) {
    -  $variables['content_attributes']['class'][] = 'content';
    +
     }
    

    why leaving an empty line here?

  2. +++ b/core/modules/node/templates/node.html.twig
    @@ -75,27 +76,31 @@
    -  {% if display_submitted %}
    -    <footer>
    -      {{ user_picture }}
    -      <p class="submitted">{{ submitted }}</p>
    +  {% if author or display_submitted %}
    +    <footer class="node__meta">
    +      {{ author }}
    +      {{ submitted }}
    

    aren't we changing the behavior here? the new if statement means that if we have author and not display_submitted, we will still display the submitted and author, looks like a regression to me.

Regarding $variables['content_attributes'], it was added to node and other entity templates in D7 for consistency in order to have:
- $variables['attributes'] for the element wrapping the entity output,
- $variables['title_attributes'] for the element wrapping the title, and
- $variables['content_attributes'] for the content wrapping element.
See block.html.twig for an example of where these are placed in the output. The RDF module makes use of 'attributes' for the entity wrapper, and 'title_attributes', but not 'content_attributes'. 'content_attributes' was added to be consistent, and in case other modules would need it. I don't know if there are module which make use of content_attributes.

mortendk’s picture

Title: Markup for: node.html.twig » node.html.twig template
Status: Needs work » Needs review
Issue tags: -Novice, -sprint, -drupaldevdays +focus
FileSize
20.26 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,543 pass(es), 1 fail(s), and 0 exception(s). View
7.37 KB

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

Status: Needs review » Needs work

The last submitted patch, 124: node-2004252-124.patch, failed testing.

joelpittet’s picture

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!

camoa’s picture

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.

Jeff Burnz’s picture

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.

mortendk’s picture

Status: Needs work » Needs review
FileSize
20.87 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,310 pass(es). View

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

mortendk’s picture

Issue summary: View changes
Jeff Burnz’s picture

mortendk’s picture

@jeff awesome - lets clean that out :)

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

joelpittet’s picture

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.

Jeff Burnz’s picture

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.

joelpittet’s picture

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

mgifford’s picture

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.

Jeff Burnz’s picture

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

mortendk’s picture

FileSize
22.96 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,573 pass(es), 1 fail(s), and 0 exception(s). View
6.4 KB

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)

mgifford’s picture

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

mortendk’s picture

@mgifford cool so it seems that we agree :)

mgifford’s picture

Yup. Looking forward to seeing this in Core.

mortendk’s picture

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

mortendk’s picture

yo bot get to work ..

joelpittet’s picture

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.

mortendk’s picture

FileSize
7.58 KB
23.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,574 pass(es). View

heres more for you joel

The last submitted patch, 138: node-2004252-136.patch, failed testing.

joelpittet’s picture

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.

mortendk’s picture

FileSize
22.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,692 pass(es). View
4.54 KB

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

camoa’s picture

Die ID, Die!!

joelpittet’s picture

FileSize
24.54 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,709 pass(es). View
3.62 KB

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

mortendk’s picture

Issue summary: View changes
mortendk’s picture

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

joelpittet’s picture

Issue tags: +Needs manual testing, +Novice

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

mortendk’s picture

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.

joelpittet’s picture

FileSize
27.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,680 pass(es), 5 fail(s), and 20 exception(s). View
8.16 KB

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.

Status: Needs review » Needs work

The last submitted patch, 155: 2004252-twig-node-cleanup-155.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
27.27 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,626 pass(es). View
2.76 KB

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

mortendk’s picture

Issue tags: +Needs screenshots
+++ 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 ;)

derheap’s picture

About the author_picture:

but maybe that should be a seperate issue...

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

Jeff Burnz’s picture

  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.

Jeff Burnz’s picture

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

mdrummond’s picture

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.

mortendk’s picture

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.

joelpittet’s picture

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

mortendk’s picture

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.

Jeff Burnz’s picture

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]

derheap’s picture

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

Jeff Burnz’s picture

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

scor’s picture

@joelpittet #155: "I moved the RDF stuff to the template for both the author_attributes and metadata." <-- looks good, thanks!

mortendk’s picture

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.

mdrummond’s picture

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.

mortendk’s picture

FileSize
2.66 KB
27.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2004252-node-171.patch. Unable to apply patch. See the log in the details link for more information. View

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

mdrummond’s picture

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.

mortendk’s picture

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

derheap’s picture

Status: Needs review » Reviewed & tested by the community

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.

mdrummond’s picture

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

Jeff Burnz’s picture

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.

mortendk’s picture

mortendk’s picture

Issue tags: +Stark, +Bartik
Related issues: +#2254163: node stark naked
mortendk’s picture

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

mortendk’s picture

joelpittet’s picture

@mortendk re: #172 the {{ metadata }} is for RDF. It's like like {{ title_suffix }} variable but actually named for what it contains;-)

The reason it's needed is RDF was appending metadata to the {{ submitted }} variable which no longer exists. So I needed to create a new variable to append that data to.

Thanks again for all the screenshots!:)
I'm cool with the stark 'look' different because it's no longer a <p> tag so that is expected and all the others look perfect from my quick review.

So RTBC++ continues:)

camoa’s picture

I agree with Morten, after so long, stop the patch for tiny details seems to be impractical. There is so much to argue in both sides of the coin, class or not class for title, but the big picture is: have a better node template.

I think that discussion in the latest 10 post or so goes against a lean approach to the topic. We need to know we have a good MVP here. Will it need more work as it is used in the future? most likely!! but right now, it needs to be released and tested.

Send that little bugger to commit morten, if anyone has a concern about a detail, there are ways to do it set in place(follow ups) but those are part of interaction that anyway should follow any patch release.

Jeff Burnz’s picture

#183, the devil is always in the details. It's not really about a class here or there, it's a broader discussion about being consistent or not (either way, super clean or not), it was agreed to have that later.

mortendk’s picture

Yup its about looking at the picture of ok so we are here now with the node - its a huge improvement of what core is now.
As jeff rightly points out, theres details that we need to be aligned about. Those details are there for moved to 2 other issues:
#2254163: node stark naked
#2254153: Move node classes out of preprocess and into templates
and the what do we do about the author picture :
#2247677: User picture improvements in node template

Jeff Burnz’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/node/node.module
@@ -663,23 +663,17 @@ function template_preprocess_node(&$variables) {
+  $variables['author_attributes'] = new Attribute();

What I've found is that when you do this without setting class as an empty array you will get a notice if you later attempt to add a class (e.g. in your themes preprocess_node function). Indeed, this is what is happening to me, for example when I do this: $variables['author_attributes']['class'][] = 'node__author'; I get a notice (and the class does not print):

Notice: Indirect modification of overloaded element of Drupal\Core\Template\Attribute has no effect in...(my preprocess function snippet as above)

What I've been doing when setting up new Attribute() in preprocess is this: $variables['author_attributes'] = new Attribute(array('class' => array()));

A quick discussion about whats going on here? Do we have a bug deeper in the Attribute system (different issue?). Right now, as it stands with this patch I can't add a class to this attribute.

I'm real hesitant to set this back to needs work but this does not work properly AFAICT unless I am doing something way wrong here, which I'm pretty sure I'm not.

joelpittet’s picture

@Jeff Burnz I'm inches away from making that way way easier to deal with here: #2239945: Refactor Attribute class into one class

I also tried special casing 'class' which will work kinda but there are a couple of other attributes that use AttributeArray like 'class' does. Though I prefer that refactor one. I'd love is someone could take a stab or just tag team that issue with me, even if it's just a small brainstorm session on skype or something.

You are correct though, you do need to initialize class but you can do it like this too:

$variables['author_attributes'] = new Attribute();
$variables['author_attributes']['class'] = array();

Detailed in the change record @see https://drupal.org/node/1727592

ArrayAccess::OffsetSet() deals with the AttributeArray instanciation at the moment.

Jeff Burnz’s picture

Yep, I understand what you mean here, my main concern here is/was is that we are actually printing {{ author_attributes.class }} in the template, but you would have to guess that you need to initialise this yourself before you can use it, or find the change notice (which I did some time ago but actually forgot about it). My point was that new users will face a PHP notice for using something they think should just work based on what they're reading in the template.

From my POV I can let this slide and set it back to RTBC, since I know to do this, or is this rather bad TX, or just the way it is - I'm not strongly favourable one way or the other. If it can be fixed, which is what I think you're suggesting in #2239945: Refactor Attribute class into one class, then we can move on.

joelpittet’s picture

Speaking of TX/DX and attributes, I need someone to review this patch #2108771: Remove special cased title_attributes and content_attributes for Attribute creation
This avoids treating content_attributes and title_attributes variables different then all other attributes that are built out in the theme system. I tried to do $variables['attributes'] too which is also a special cased variable but that turned out to be a bigger challenge and if I can get those other two committed I can make a followup to do the $variables['attributes'] as well.

*shameless attributes plug*

Jeff Burnz’s picture

+++ b/core/modules/node/templates/node.html.twig
@@ -75,27 +77,34 @@
+  {% if content.links %}
+    <div class="node__links">
+      {{ content.links }}
+    </div>
+  {% endif %}

I'm finding this is not always empty, e.g. in display mode full there can be no links but there are items in the array, upshot - empty markup:.

mortendk’s picture

oooh damn

Jeff Burnz’s picture

damn indeed, is that something new, did this array suddenly change or something, I don't recall it being like that before (surely one of us would have noticed this), I just noticed it in my own template and went, oh shit, what-t/f is that?

markcarver’s picture

Correct. This should be: {% if content.links is not empty %}. Twig can handle checking if it's empty appropriately.

Jeff Burnz’s picture

I don't think it's that easy Mark, because this uses a render-cache-placeholder, content.links appears to be never empty.

markcarver’s picture

Status: Needs work » Reviewed & tested by the community

Render cache placeholders are a separate issue then and should be just another follow-up issue that needs to be created. Setting back to RTBC since Attributes already have their own issue as well.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

This issue still has the tag "Needs issue summary update". Also this change will impact themers so we need a change record for them.

According to the comments we need followup for:

  • #124 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.
  • #163 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. #2254153: Move node classes out of preprocess and into templates
  • #172 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. ?
  • #174 A cleaner stark (remove all node--foo classes etc)
  • #174 picture article fun as jeffburnz created. #2247677: User picture improvements in node template
  • #174 an epic bartik 2.0

As far as I can tell only 2 of them have issues actually created.

I'm conscious that this has been rtbc for a long time so once the change record has been created, the issue summary checked and followups exist this will go in.

alexpott’s picture

More followups :)

  • #171 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.)
  • #195 Render cache placeholders are a separate issue then and should be just another follow-up issue that needs to be created.
LewisNyman’s picture

Issue tags: +frontend
mortendk’s picture

Added followups:
171: nodeteaser classes #2277905: node teaser title classname
195: render cache: #2277909: node render cache
172: meta bartik, stark & drupalbase #2277913: meta: the role of stark & bartik in Drupal8 twig (also takes on the cleaner stark, bartik2 etc)

did i miss anyone of em ?

alexpott’s picture

172: 2004252-node-171.patch queued for re-testing.

The last submitted patch, 172: 2004252-node-171.patch, failed testing.

sarahjean’s picture

Assigned: mortendk » sarahjean

Working on change record at the DrupalCon contrib sprint

mortendk’s picture

im in the sprint room come hunt me down if you need an overview of what actually happend

crowdcg’s picture

Starting the change record while sarahjean works on the re-roll.

sarahjean’s picture

Status: Needs work » Needs review
FileSize
27.38 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,789 pass(es). View

Reroll of the patch from #172

sarahjean’s picture

Assigned: sarahjean » Unassigned
Issue tags: -Needs change record

Finished up change record and patch reroll.

mikl’s picture

Status: Needs review » Reviewed & tested by the community

Great work, looked it over, this will make it much cleaner to work with :)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed b001084 and pushed to 8.x. Thanks!

camoa’s picture

YAYYY!!! Y'all make it happen!!!

yched’s picture

So this did the following in node.html.twig :

-<article id="node-{{ node.id }}" class="{{ attributes.class }} clearfix"{{ attributes|without('id', 'class') }}>
+<article{{ attributes }}>

#2217731: Move field classes out of preprocess and into templates is about doing the exact opposite in field.html.twig :

-<div{{ attributes }}>
+<div class="field field-{{ entity_type }}--{{ field_name }} field-name-{{ field_name }} field-type-{{ field_type }} field-label-{{ label_display }}{{ (attributes.class is defined) ? ' ' ~ attributes.class  : '' }}"{{ attributes }}>

Can someone from this issue here chime in over there about the global strategy for "attributes & classes in templates" ?

mortendk’s picture

@yched there has been a lot of confusion about this & frankly its because we didnt have an agreed upon strategy
Theres a followup issue right now on the node.html.twig file that is trying to clean this up.

At drupalcon in austin we finally got a plan for the whole class mess:
global stategy is that all class generating will be moved out of preprocess and into the templates, so the themes dont have to work around preprocess & all that jazz
joel pittet is working on a POC that can make it happen, theres a first step for this here : #2254153: Move node classes out of preprocess and into templates

yched’s picture

@mortendk: Makes sense. Thanks for the update :-)

Status: Fixed » Closed (fixed)

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