Markup for: comment.html.twig

Since we're being HTML5 aware, we probably should add a <header tag as well:

<article{{ attributes }}>

<header>
  {{ title.prefix }}
  {% if new %}
    <mark class="new">{{ new }}</mark>
  {% endif %}
  <h3{{ title.attributes }}>{{ title.label }}</h3>
  {{ title.suffix }}
</header>

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

    {#
      Indicate the semantic relationship between parent and child comments
      for accessibility. The list is difficult to navigate in a screen
      reader without this information.
    #}
    {% if parent %}
      <p class="parent visually-hidden">{{ parent }}</p>
    {% endif %}

    {{ permalink }}
  </footer>

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

    {% if signature %}
      <div class="user-signature">
        {{ signature }}
      </div>
    {% endif %}
  </div>

  {{ links }}

</article>

There was some discussion over in #1965650: Comment rendering broken: comment body rendered outside of the bubble to improve Bartik's comment markup to use author and datetime elements. Also note that core comment.html.twig has the mark element and Bartik's does not.

The markup developed in that issue for Bartik was going to be:

<article class="{{ attributes.class }} clearfix"{{ attributes }} role="article">

  <header>
    <div class="attribution">
      {{ user_picture }}

      <div class="submitted">
        <address>
          {{ author }}
        </address>
        <time>
          {{ created }}
        </time>
        <p class="comment-permalink">
          {{ permalink }}
        </p>
        {#
          // Indicate the semantic relationship between parent and child comments
          // for accessibility. The list is difficult to navigate in a screen
          // reader without this information.
        #}
        {% if parent %}
        <p class="comment-parent visually-hidden">
          {{ parent }}
        </p>
        {% endif %}
      </div>
    </div> <!-- /.attribution -->
  </header>

  <div class="comment-text">
    <div class="comment-arrow"></div>

    {% if new %}
      <mark class="new">{{ new }}</mark>
    {% endif %}

    {{ title_prefix }}
    <h3{{ title_attributes }}>{{ title }}</h3>
    {{ title_suffix }}

    <div{{ content_attributes }}>
      {# We hide the links now so that we can render them later. #}
      {% hide(content.links) %}
      {{ content }}
    </div> <!-- /.content -->

    <footer>
      {% if signature %}
      <div class="user-signature clearfix">
        {{ signature }}
      </div>
      {% endif %}

      <nav>
        {{ content.links }}
      </nav>
    </footer>
  </div> <!-- /.comment-text -->

</article>

Now comment settings is a field, also realted #1962846: Use field instance name for header of comment list, drop comment-wrapper template

author view mode

Don't mind me, just getting rid of the hash on the dreammarkup tag to keep things consistent, and D7 d.o might have some issues with tags with '#' in them as well.

I don't understand if this proposed markup has to be implemented just as it is, because the actual markup contains stuff like hiden class on mark new tag that would be lost... And the new view_mode bio doesn't exists yet... Maybe this issue is blocked by other by the moment...? @andypost can you give more feedback about that please? Thank you!

"new mark" now added via JS
view mode seems makes no sense now

Suppose better to start from #1962846: Use field instance name for header of comment list, drop comment-wrapper template

took a stap at the comments and cleaned up all the classes thats added in & put them into the template

1. +++ b/core/modules/comment/comment.module
   @@ -1411,28 +1411,9 @@ function template_preprocess_comment(&$variables) {
   -  $variables['attributes']['class'][] = 'comment';
   

   removed the hardcoded class & added it into the template

2. +++ b/core/modules/comment/comment.module
   @@ -1411,28 +1411,9 @@ function template_preprocess_comment(&$variables) {
   -    $variables['attributes']['class'][] = $variables['status'];
   

   status is allready available for the template so its created inside the template

3. +++ b/core/modules/comment/comment.module
   @@ -1411,28 +1411,9 @@ function template_preprocess_comment(&$variables) {
   -    $variables['attributes']['class'][] = 'by-anonymous';
   

   is this ever an issue anybody have used for anything ? removed cause the rule of theming for the 80% usecases

4. +++ b/core/modules/comment/comment.module
   @@ -1411,28 +1411,9 @@ function template_preprocess_comment(&$variables) {
   -      $variables['attributes']['class'][] = 'by-' . $commented_entity->getEntityTypeId() . '-author';
   -    }
   

   again for the 80% usecases

5. +++ b/core/modules/comment/comment.module
   @@ -1411,28 +1411,9 @@ function template_preprocess_comment(&$variables) {
   -  $variables['attributes']['class'][] = 'clearfix';
   

   die

6. +++ b/core/modules/comment/comment.module
   @@ -1453,11 +1434,6 @@ function template_preprocess_comment_wrapper(&$variables) {
   -  $variables['attributes']['id'] = 'comments';
   
   +++ b/core/modules/comment/templates/comment-wrapper.html.twig
   @@ -35,17 +35,17 @@
   -    <h2 class="title">{{ 'Comments'|t }}</h2>
   

   put into the template

Start testing...

Altered the patch so the CommentCSSTest.php would no longer include this:

        // Verify the by-anonymous class.
        $comments = $this->xpath('//*[contains(@class, "comment") and contains(@class, "by-anonymous")]');
        if ($case['comment_uid'] == 0) {
          $this->assertTrue(count($comments) == 1, 'by-anonymous class found.');
        }
        else {
          $this->assertFalse(count($comments), 'by-anonymous class not found.');
        }

        // Verify the by-node-author class.
        $comments = $this->xpath('//*[contains(@class, "comment") and contains(@class, "by-node-author")]');
        if ($case['comment_uid'] > 0 && $case['comment_uid'] == $case['node_uid']) {
          $this->assertTrue(count($comments) == 1, 'by-node-author class found.');
        }
        else {
          $this->assertFalse(count($comments), 'by-node-author class not found.');
        }

Because those 2 classes are no longer used.

Also changed CommentPreviewTest.php from:

    $this->assertFieldByXPath("//article[contains(@class, 'preview')]//div[contains(@class, 'user-picture')]//img", NULL, 'User picture displayed.');

to:

    $this->assertFieldByXPath("//article//div[contains(@class, 'user-picture')]//img", NULL, 'User picture displayed.');

Still a todo:
Please note, we need to keep the variables in sync with the node template:
#2004252: node.html.twig template

Suppose it's time to think a bit about comments formatter too, without a big picture how comments would be rendered this issue will cause another major to properly work with render cache.
The intermediate level of render it's views blocks and formatter widget, also comment should be able to be JSON serialization into frontend themer (suppose a backbone template).
Currently we have a modal piece of render (comment entity list + comment form) so comment wrapper needs some love too.
Also entity link can open modal with comment list...

I suppose wrapper should gone in favor of formatter template or render function with caching
And other links to comments should work through #2002094: Improve performance of comment.html.twig

1. +++ b/core/modules/comment/comment.module
   @@ -1453,11 +1434,6 @@ function template_preprocess_comment_wrapper(&$variables) {
   -  $variables['attributes']['class'][] = 'comment-wrapper';
   
   +++ b/core/modules/comment/templates/comment-wrapper.html.twig
   @@ -35,17 +35,17 @@
   -<section{{ attributes }}>
   +<section class="comment-wrapper {{ attributes.class }}" {{ attributes }} id="comments">
   

   should gone a whole.

2. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentCSSTest.php
   @@ -93,24 +93,6 @@ function testCommentClasses() {
   -          $this->assertTrue(count($comments) == 1, 'by-anonymous class found.');
   ...
   -          $this->assertFalse(count($comments), 'by-node-author class not found.');
   

   is there equivalent in twig template?

3. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentPreviewTest.php
   @@ -71,7 +71,7 @@ function testCommentPreview() {
   -    $this->assertFieldByXPath("//article[contains(@class, 'preview')]//div[contains(@class, 'user-picture')]//img", NULL, 'User picture displayed.');
   +    $this->assertFieldByXPath("//article//div[contains(@class, 'user-picture')]//img", NULL, 'User picture displayed.');
   

   This check needed to ensure that preview have user picture so selector is wrong

4. +++ b/core/modules/comment/templates/comment-wrapper.html.twig
   @@ -35,17 +35,17 @@
   -    <h2 class="title">{{ 'Comments'|t }}</h2>
   +    <h2>{% trans %}Comments{% endtrans %}</h2>
   

   This make sense but there's special field instance setting

5. +++ b/core/modules/comment/templates/comment-wrapper.html.twig
   @@ -35,17 +35,17 @@
   -    <h2 class="title comment-form">{{ 'Add new comment'|t }}</h2>
   +    <h2 class="title comment-form">{% trans %}Add new comment{% endtrans %}</h2>
   

   any actual purpose to change to this kind of translation?
   Also now we have entity operations #2165725: Introduce hook_entity_operation()

this patch should fix the classnames to follow the code standards

bot get to work

+++ b/core/modules/comment/templates/comment.html.twig
@@ -75,20 +74,20 @@
     {% if parent %}
-      <p class="parent visually-hidden">{{ parent }}</p>
+      <div class="comment__parent visually-hidden">{{ parent }}</div>
     {% endif %}

We are trying to minimize this usage in #1857946-11: Comment parent template variables are built twice

We should also be looking at the template suggestions after #2228763: Create a comment-type config entity and use that as comment bundles, require selection in field settings form see #2282205: Revisit comment template suggestions

Patch outdated after #1962846: Use field instance name for header of comment list, drop comment-wrapper template

+++ b/core/modules/comment/templates/comment-wrapper.html.twig
@@ -35,17 +35,17 @@
-<section{{ attributes }}>
+<section {{ attributes }} id="comments">

there could be more then one comment fields on page now

I'm going to test this one.

Please make sure about changes in related #1857946: Comment parent template variables are built twice

Unassigning for now.

I can see that proposed markup in issue summary is much different then markup in discussion later.
For now, it's not clear what to do. Can you please update issue summary so someone can start/continue/finish this issue as well?

IS should be updated still, but here's a result of discussions within Goa code sprint

in related issue comment links are moved to comment content

Looking forward to @mortendk's review :)

+++ b/core/modules/comment/templates/comment.html.twig
@@ -68,24 +68,19 @@
 <article{{ attributes }}>
-  {% if title %}
-    {{ title_prefix }}
-  {% endif %}
-
...
-
   {% if title %}
+    {{ title_prefix }}
     <h3{{ title_attributes }}>{{ title }}</h3>
     {{ title_suffix }}
   {% endif %}

LOL! WTF, D8 HEAD!?

Fixed some Twig comments. Also pinged @mortendk to have his approval ;)

Re #33

+++ b/core/modules/comment/templates/comment.html.twig
@@ -68,49 +68,44 @@
   <mark class="hidden" data-comment-timestamp="{{ new_indicator_timestamp }}"></mark>
-
   {% if title %}
+    {{ title_prefix }}

My guess is that mark tag was supposed to be after the prefix though shows if the title is missing too. Though not sure if that is needed structure or not, i'd have to guess not if it works without a title too.

Once links goes to content and #1548204: Remove user signature and move it to contrib then I see no reason in footer, also we need to care about "threading" (div indents) #2318579: Remove comment_prepare_thread()

Comment always have a subject and in case of absense of comment body the subject would be (No subject)

User signatures gone #1548204: Remove user signature and move it to contrib

Patch rerolled

Thanks @epari.siva, the patch applies but the patch needs to be updated:

+++ b/core/modules/comment/templates/comment.html.twig
@@ -67,43 +67,44 @@
+  {% if signature %}
+    <footer>
+      {{ signature }}
+    </footer>
   {% endif %}

+++ b/core/themes/bartik/templates/comment.html.twig
@@ -110,6 +112,11 @@
+    {% if signature %}
+      <footer class="comment__footer user-signature clearfix">
+        {{ signature }}
+      </footer>
+    {% endif %}

+++ b/core/themes/classy/templates/content/comment.html.twig
@@ -73,43 +73,44 @@
+  {% if signature %}
+    <footer class="user-signature">
+      {{ signature }}
+    </footer>
   {% endif %}

These bits need to be removed.

Suggested changes included.

Rerolled, as last patch contained references to User signature, but it's gone according to #1548204: Remove user signature and move it to contrib

+++ b/core/themes/bartik/templates/comment.html.twig
@@ -110,6 +112,6 @@
-    </footer>
+    </div>

this change is wrong, it's footer closing tag

Patch updated with @andypost comment.

@Cottser now it looks ready

Now we need update summary and RTBC

And how it looks with patch in standard profile - bartik

No interdiff because here's a code merged in from #2428509: Comment links weight should be managed by view display

1) updated summary with latest module provided markup
2) added screenshots before-after
3) .comment-footer unused selector (it was also) so maybe more clean-up possible
4) .comment__text h3 is needed to remove default H3 1em

@todo #49 data type still needs debug

Also I think we need to swap comment__text and comment_content

PS: I mean code merged from #2428509: Comment links weight should be managed by view display

I removed some no needs markup

RTL styles broken #2480497: Comment rtl styling broken . Fixed with patch.

@zestagio commenting in [rtl] is looks good for me... Also i have one concern, How do we fit it in small screens ?

I had captured a screenshot in small screen, Input field is getting disturbed in it. Can you please check the screenshot.

@Manjit.Singh can you explain "disturbed" in what way. The screenshot looks fine to me for that input you highlighted.

Bump. Looks like this is actually ready?

We need issue summary update

+++ b/core/themes/bartik/templates/comment.html.twig
@@ -73,43 +73,37 @@
+  <span class="hidden new" data-comment-timestamp="{{ new_indicator_timestamp }}"></span>

if the new class is getting modifed or used by javascript we should call it "hidden js-new new" according to the js & css issue, afaik the class is expected to be modied by js

so thers a couple of issues here:
* the use of header is not correct that should be a <footer> instead just as we doing in node.html.twig
(if theres a need for bikeshed look at https://developer.mozilla.org/en-US/docs/Web/HTML/Element/aside & https://developer.mozilla.org/en-US/docs/Web/HTML/Element/header + the node template discussion on using <footer>: #2004252: node.html.twig template)
* the js used classes needs prefixing & the new class should be removed from bartik, to keep it the same.
* .comment__meta is added as well to keep it aling with node.html.twig

I think i fixed it all but it need screenshots + test & issue summary

updated the issue summary

Looks like a nice clean-up, here is a quick pass at a review:

1. +++ b/core/modules/comment/js/node-new-comments-link.js
   @@ -109,7 +109,7 @@
   -            .removeClass('hidden');
   +            .removeClass('js-hidden hidden');
   

   Any chance the hidden should stay there and just remove the js-hidden?

2. +++ b/core/modules/comment/templates/comment.html.twig
   @@ -66,32 +66,27 @@
   +{%
   +  set classes = [
   +    'js-comment',
   +  ]
   +%}
   +<article{{ attributes.addClass(classes) }}>
   

   With one class it would be nicer to just put it in the addClass method, also more performant as it doesn't have to create a local variable.

3. +++ b/core/modules/comment/templates/comment.html.twig
   @@ -66,32 +66,27 @@
   +  <mark class="hidden js-hidden" data-comment-timestamp="{{ new_indicator_timestamp }}"></mark>
   

   Don't you want to remove the 'hidden' class from there?

4. +++ b/core/themes/bartik/css/components/comments.css
   @@ -57,7 +54,11 @@
   +.comment__content h3 {
   +  /*line-height: 1.7;*/
   +  margin-top: 0;
   

   Oops? Left over comment?

.hidden we use .hidden as a class in drupals core css, and thats where the magic happens so we should keep that class

addClass('js-comment') true gonna fix
... i dunno what goes on in bartik but yes *burn* ;)

fixed as comment on #66

and interdiff *sorry*

All the comments by @joelpettit at #65 appear to have correctly been implemented.

I have added some comments to the article content type and it does appear to be creating good output. See screenshot:

I think it's good to go.

Sorry I'm still a bit confused what we are doing here?

1. +++ b/core/modules/comment/js/comment-new-indicator.js
   @@ -59,9 +59,9 @@
   -          .removeClass('hidden')
   +          .removeClass('js-hidden')
   

   Here's where it's confusing. The hidden class has the functionality of show/hide an element. (presumably)

   And here we are replacing it with js-hidden, which doesn't show/hide the element. Which basically makes this break?

2. +++ b/core/modules/comment/js/node-new-comments-link.js
   @@ -109,7 +109,7 @@
   -            .removeClass('hidden');
   +            .removeClass('js-hidden hidden');
   

   Then here we are removing js-hidden and the hidden class. Why not just js-hidden in this case?

Thanks for the screenshots @rachel_norfolk

Because this is JS functionality it would be nice to show that (not sure what tool people are using for those cool little animated gif screencasts but that would be nice here)

I'll see if I can show it in an animation. (I'm a whizz with ffmpeg ;-) )

As I understand it, the initially outputted html does not show the "new" marker on new tweets as that would break the cache for the page. We use js to work it out and then show/hide as necessary per comment. This does make it seem weird on first output, though.

Aha - it appear you were right to be confused, @joelpettit!

The idea of the comment-new-indicator.js is to search for the appropriate new indicators and switch them on by removing the hidden class (as well as adding the correct "new" text). Trouble is; we were removing the js-hidden class which is there simply as a handle for js to find the location more easily - it shouldn't ever be removed.

So, attached is a tiny, tiny patch to make that correction.

Also, a fancy gif...

#72 was exactly my concern, and it fully addressed it :) Thanks, @rachel_norfolk :)

(I brought the JS-based "new" indicator to core — before it was utterly and completely breaking cacheability.)

But, given that, I have my doubts about some of the changes in this area. Apologies if this is documented clearly somewhere — please point me to the docs if I'm mistaken.

1. +++ b/core/modules/comment/js/node-new-comments-link.js
   @@ -109,7 +109,7 @@
   +            .removeClass('js-hidden hidden');
   
   +++ b/core/modules/comment/templates/comment.html.twig
   @@ -66,32 +66,22 @@
   +  <mark class="hidden js-hidden" data-comment-timestamp="{{ new_indicator_timestamp }}"></mark>
   

   Does this really need to remove both of these classes?

   It does, but only because both classes are added in the Twig template. But the hidden class is there really for the CSS associated with it, not for the JS to discover it. So shouldn't it therefore be just hidden?

2. +++ b/core/themes/bartik/templates/comment.html.twig
   @@ -73,43 +74,37 @@
   +  <span class="hidden js-hidden" data-comment-timestamp="{{ new_indicator_timestamp }}"></span>
   

   Same here.

3. +++ b/core/themes/classy/templates/content/comment.html.twig
   @@ -66,6 +66,7 @@
        'comment',
   +    'js-comment',
   

   js-comment is used as a selector, so therefore it is justified, unlike js-hidden.

4. +++ b/core/themes/classy/templates/content/comment.html.twig
   @@ -73,31 +74,21 @@
   +  <mark class="hidden js-hidden" data-comment-timestamp="{{ new_indicator_timestamp }}"></mark>
   

   Again.

ok look like i was overthinging it when i dropped in js-hidden ;)
removed .js-hidden it again

Perfect :)

+1 to RTBC

+++ b/core/themes/bartik/templates/comment.html.twig
@@ -73,43 +74,37 @@
+    {{ content|without('links') }}
+    {% if content.links %}
+      <nav>{{ content.links }}</nav>
+    {% endif %}

my only concerns about this part in context of #2455211: Comment field displayed last regardless of assigned weight
but this out of scope because requires new preprocess function in bartik

@andypost - yes, that will need to be addressed in #2455211: Comment field displayed last regardless of assigned weight (which will probably need a re-roll once this is committed)

RTBC+1

I *think* it was just a testbot having a moment. Will keep an eye for the result of a re-submit...

shakes fist at testbot

after testbot got sober im gonna set it back to RTBC as it was in #77

There are some pretty big visual changes with this patch...

Before

After

CSS class was not updated in comments.css , so there was some regression issues.

Updating classes and adding css. Please verify the screenshots as well.

There appears to be a few differences between the screenshots, both those applied and ones I've recreated. Taking a look...

I've adjusted the font-size, line-heights and margins to now visually match current HEAD really quite closely.

1. +++ b/core/themes/bartik/css/components/comments.css
   @@ -29,27 +30,27 @@
   -.comment__author .username {
   +.comment__meta__author .username {
   ...
   -.comment__submitted__data {
   +.comment__meta__author {
   ...
   -.comment__time {
   +.comment__meta__time {
   ...
   -.comment__permalink {
   ...
   +.comment__meta__permalink {
   

   I think I prefer the simpler classes like .comment__author in stead of .comment__meta_author

2. +++ b/core/themes/bartik/css/components/comments.css
   @@ -57,7 +58,12 @@
   +.comment__content h3 {
   +  font-size: 1.175em;
   +}
   

   We should avoid changing the heading styling in different locations and instead use the heading classes introduced in #2400233: Add reusable heading classes to Bartik

3. +++ b/core/themes/bartik/css/components/comments.css
   @@ -97,10 +103,12 @@
   +.comment ul.links,
   +[dir="rtl"] .comment ul.links {
   

   Do we definitely need to include the rtl selector here as well? Also can we remove the ul from the selector?

4. +++ b/core/themes/bartik/css/components/comments.css
   @@ -97,10 +103,12 @@
    .comment ul.links li {
   ...
    [dir="rtl"] .comment ul.links li {
   

   Can we also use the uls from these selectors?

+++ b/core/themes/bartik/css/components/comments.css
@@ -29,27 +30,27 @@
-.comment__author .username {
+.comment__meta__author .username {

@lewis so if were keeping to the bem naming its correct to use comment__meta__author, even that the name gets a bit longer (one of the pitas on bem)
If we choose to ignore bem (which i think would be a bad idea) then we should discuss dropping comment__meta as a wrapper - Its there to keep us aligned with the node.html.twig node__meta.
I think its important that we look at the overall classes & structure for the whole node (as were expecting comments to be there) - how they are aligning up towards eachother with classnames, so we dont end up having different concepts.

... edit:
looking at it, its what bartik does so it kinda can do whatever right ?

Now then. With reference to #93

1. Implemented - it seems sensible.
2. Implemented. This will mean there is a tiny visual difference in screenshots from 8.0.x but anything that encourages a move to rem isn't a bad thing ;-)
3. Implemented
4. Implemented.

Base 8.0.x:

After patch:

@rachel_norfolk thanks for working on this.. There are some minor differences in titles's font size as per your screenshots.

Sorry Manjit, I should have explained our motivation around the agreed visual difference further in my comment...

You are correct that the h3 title in the comment is not exactly identical to that in the current 8.0.x HEAD. I did adjust it at #91 but, whilst sat with the Bartik maintainer at Frontend United this weekend, we agreed with #93 that it was more important NOT to redefine h3 within the comment css.

Once this patch is complete, we are planning to introduce another patch to redefine how the font-size is declared in Bartik. That will make the difference MUCH easier to cancel out.

I'm setting this back to In Review, if that's okay?

I'm hanging around in IRC #drupal-twig if you want to chat about it?

+1 on RTBC the maintainer of bartik do have knowledge about whats happening & theres a plan then lets move forward (else create a followup issue :)

There is some visual changes on Bartik because of the patch makes comments use the semanticly right html elements but it doesn't look very major or something we should be worried about. The maintainer of Bartik is also fine with this, so am I. Patch generally seems to be working, there is some screenshots showing the visual regression and patch looks clean so I think this is RTBC!

+++ b/core/themes/bartik/templates/comment.html.twig
@@ -77,43 +78,37 @@
+  <footer class="comment__meta">
+    {{ user_picture }}
+    <p class="comment__author">{{ author }}</p>
+    <p class="comment__time">{{ created }}</p>
+    <p class="comment__permalink">{{ permalink }}</p>

In #94 @mortendk pointed out that this was breaking BEM standards - it should be comment__meta__author etc. And in IRC @LewisNyman said that "Bartik should also follow BEM standards... eventually.".

So I think that when introducing new classes or changing things we should try to follow BEM. Setting to needs review and asking for @emma.maria's opinion.

In #94 @mortendk pointed out that this was breaking BEM standards - it should be comment__meta__author etc.

Actually the current patch is correct. It shouldn't be comment__meta__author

+++ b/core/themes/classy/templates/content/comment.html.twig
@@ -77,31 +78,21 @@
-    <p class="submitted">{{ submitted }}</p>
+    <p class="comment__meta__submitted">{{ submitted }}</p>

I'm setting this back to needs work because this markup is incorrect, this should be comment__submitted and not comment__meta__submitted

I am aware that this issue is assigned to me. I will take a closer look at this, this evening.

+++ b/core/themes/bartik/templates/comment.html.twig
@@ -77,43 +78,37 @@
+  <footer class="comment__meta">
...
+    <p class="comment__author">{{ author }}</p>

@lewis i have to disagree with you on this ;)
As i read the class organization .comment as the main element, so .comment__meta should have .comment__ to show its inheritence and keep same naming convention as .node__meta.
The sub element of .comment__meta cant just be named should then be named .comment__meta__author, they shuld have the comment__meta__[foo] else the naming & predictability have gone lost.
ex: .comment__meta__time, .comment__meta__permalink

If we dont want to have multiple groupings ex: .foo__bar__baz as classnames, we should to add that to https://www.drupal.org/coding-standards/css/architecture#formatting and we should then instead name the basecalss .comment-meta (.comment-meta__author, .comment-meta__time, ,.comment-meta__permalink

@mortendk An important part of OOCSS is that the class names should not follow the HTML structure. It should matter that .comment__author is inside of .comment__meta. It makes no difference, because it should look the same regardless of where it is placed in the HTML structure.

This isn't really a strict rule, that's why it is not forbidden in our standards, there is also no suggestion of one. We should avoid it unless there is a reason why the styling of .comment__author relies on the styling of .comment__meta, like padding or position: relative or something? I don't see that here though.

@lewis yes its an important part of oocss to split elements up, its also an important way of documenting where an element lives.
When theres a naturel connection between elements the classname should reflect that else theres basically no need at all for .parent__kiddo naming.
Were just using the modifers .foo & .foo--modifier instead.

My reason for this on the meta is that i compare comments meta the same way as the "media object"

.media {...}
.media__image {...}
.media__text {...}

Thats in principal the same we do with the comments meta, why im suggesting .comment-meta__authorname, as its part of for a comment, and we should update the documentation for this, so we dont end up with .comment__meta__autorname__lastname--somethingclasses

@mortendk Yeah we can update the coding standards. I just want to understand the reasoning behind .comment-meta__author, because I can't see why it can't just be called .comment__author, you should be allowed to move it out of .comment__meta and you should be able to move any other sub-element inside of .comment__meta without having to change the class.

cause we define all the author, time, picture etc as "comment meta" just as in the node - Why I se a logical connection between node & comment.
Im not gonna all-in on this, and we could totally do it as a followup, where we align node & comment classes & QA em, cause we are now down in very small details, that really is just stopping progress.

+1 on updating the documentation, else we can end up with .foo__bar__baz__kazoom__wtf__argh classes in contrib & we really dont want that.

+ 1 to comment__author. Author is an element in a comment, whether or not it is also under __meta. Chaining multiple __element things together gets overwhelming.

I'd also suggest going with comment__submitted rather than comment__meta__submitted for the same reasons.

+ 1 to comment__author and not chaining things. The author is an element within comment it can be moved around in any part of a comment and can keep the same class name and styles. We add variants to make things more specific not stack components in class names :)

Also now we have more consensus on markup, I will take a closer look at what has happened to Bartik.

updated the patch changed classy from .comment__meta__submitted to .comment__submitted
As well i spotted a .clearfix in classy that was floating around - it dont have anything to do there, so a little house cleaning as well.

Except few nits good to go, who should fire RTBC?

1. +++ b/core/modules/comment/templates/comment.html.twig
   @@ -102,9 +92,12 @@
      </div>
   -  {% if content.links %}
   -    {{ content.links }}
   -  {% endif %}
   +
    </article>
   
   +++ b/core/themes/classy/templates/content/comment.html.twig
   @@ -111,9 +101,12 @@
      </div>
   -  {% if content.links %}
   -    {{ content.links }}
   -  {% endif %}
   +
    </article>
   

   is this new line needed?

2. +++ b/core/themes/bartik/templates/comment.html.twig
   @@ -77,43 +78,37 @@
   +  <div{{ content_attributes.addClass('comment__content') }}>
        {% if title %}
          {{ title_prefix }}
   -        <h3{{ title_attributes }}>{{ title }}</h3>
   +      <h3{{ title_attributes }}>{{ title }}</h3>
          {{ title_suffix }}
   

   strange indent change

3. +++ b/core/themes/bartik/templates/comment.html.twig
   @@ -77,43 +78,37 @@
   +    {{ content|without('links') }}
   +    {% if content.links %}
   +      <nav>{{ content.links }}</nav>
   +    {% endif %}
   

   We manage links in UI so this would be fixed in #2428509: Comment links weight should be managed by view display anyway

nid spacing fixed, the indentation is correct & yes lets fix the links in the follow up & get this one out the door

do we need screenshots ?

Tested, all ok. Like in #95
Without patch: http://storage3.static.itmages.com/i/15/0708/h_1436369906_1757861_2a4f66...
With patch: http://storage1.static.itmages.com/i/15/0708/h_1436369580_5137667_b292d9...

Add images without link to screenshots.

With patch:

Without patch:

@HOG thanx for screens, fixed IS

Looks we need RTBC from @emma.maria

awesome then this should finally be ready to roll :)

I'm going to do some visual regression testing and check the Bartik templates right now.

Tested and found some minor visual changes, adding screenshots.

Will edit this soon and provide more information about the error

More information about last comment,

screenshot 1'bug nopatch': before using the patch, the h3 is outside the 'comment__content' class

screenshot 2 'bug patch': after patching, the h3 is inside 'comment__content' and has the font-size applied too

Hi and welcome Pablo! The visual regression you mentioned above is, as you say, due to the new structure of the html within the comment and (currently) the use of em font sizes. As mentioned in #98, we want to accept this regression for the time being and then have a follow up issue to change the em font size declarations to rems, which will remove the regression.

In the meantime, we might have to live with the tiny font size difference.

ok let's get this to rtbc so we can move further with other issues on comments.

maintainer of bartik is aware 2 times screenshots. we have a rtbc :)
pulling the tricker

I am aware that there are visual regressions and it looks like they can be fixed in this issue.

A good few people reviewing this have pointed out that there are obvious visual regressions so I think we can't just leave it like this. No one wants to raise a follow up and I am pretty sure Core committers will point out the regressions also. I am happy to take a stab at fixing this seeing as no one else wants to fix it at this point.

I took the last screenshots and compared the two, the visual progression for Bartik is 2px to the left and 3px down.

What I would like is to raise is the awareness of us becoming "our own worst client".
We stop major markup/theme progressions because of 2-3 pixels, that have no effect on the final product (Drupal8)

If Bartik was built with a major design & styleguide then yes it would make sense to build on that for everystep of the way, but its unfortunately not. Does it make sense to be "pixel perfect", when we all now that bartik was never build that way ?

We shouldn't be introducing regressions now. Drupal is a framework and a product so we shouldn't be lazy about this.

Don't we just need to change the em font size to rems to fix this problem?

That's exactly what we agreed to do at FEU - get this to RTBC and then introduce a follow up that converted to rems.

I'm very happy to help with that follow up, once we are ready to make it.

I agree with lewisnyman in some sense; if we change 1-3pxs regression in every issue, after all the issues things has visually changed quite a lot. Problem that I seen in this is that markup cannot be changed after RC is out but CSS can be still changed. Because of that fact I might still suggest to get this in even though there would be a very minor visual regression.

It seems like a really simple fix. We should do it in this issue.

I think so too. I thought CSS could be changed after RC but that wasn't true. So we should fix it here because it is not a good time to cause regressions.

@lewis & @laurii This is not about being lazy - this is about getting progress. We have way of stopping our own progress of getting basic elements in (like cleaning up a template from html4 to html5, so other issues can be fixed) We have turned into the worst possible client for ourself - And thats about until 5 minuts before RC1 and everything will be thrown in with a shovel (as it got done with ex bartik 5-6 years ago)

This is not about everything can be pushed 3pixels (we are not talking about were changing everything from blue to hotpink) its about us creating an enviroment, where we strive for a "perfection" thats not even defined. If bartik was a design that was set in stone, then it would make sense to follow those guidelines to the point - Its not and we all know that.

This rigid "Visual regression" thinking sets us in a pixel perfect state for a design thats old and worn out - If it was & we had a design manual i would not raise this issue, but it seems very silly in the comment template.

Its a huge issue if we cant fix core templates later on... If we turned RC1 in 10 minuts, then we can be sitting here talking about 3pixels, but our template in drupalcore is still html4, and we have to live with that for another X years - In that case im a pragmatic, lets fix the real stuff now and have followup's for stuff like 3px, cause the css margin-left:2px is not a part of the api but <article class="comment foo bar x"> is

So The font's have been changed overall in bartik, which is afaik not an issue that this patch should take care in this patch ??
the showstopping 2pixels of the comments boxes & the nav tag have been corrected as the screenshot should show (or compare the 2 original screenshots in your image editor or choice)

Fixed an upcoming issue with bartiks css that is not following the css documentation standards in ordering the css.

Did a new run on the fontsizes that changes cause were using ems in bartik.

Calculated values in this patch for the em values, that imho should be changed to rem, that calculating them is a pita - but would suggest this for a general followup patch in bartik.
comment__content h3

font-size:
was: 15.2880001068115px
now : 15.2300252914429px

comment__content

font-size: 
was  13.0059995651245px;
is   13.0059995651245px;
line-height: 
was 20.8095989227295px;
is : 20.8095989227295px;

These values are barely visible for the human eye, but for those that see this is an issue heres a sceenshot, as its visible even with the same line-height its not the exactly same, but i would be so bold to suggest that were on the way out on the biketrack ..

using screenshots in crome this is the comparison between the 2 files:

Tested this manually and it looks good to me. Also the comparison looks quite good; not exactly the same still but its _very_ close. Thanks morten for fixing this!

I'd be quite happy to see this one land:) Thanks for your efforts @all!
RTBC++

Any improvements could very well go into a follow-up. This gets us improvements for the markup the CSS and js class names.

Just my two cents while casually following this issue. On the subject of follow ups, if there is something that can be done in this patch you should do it here. It does not matter if there was an agreement two months ago to create a follow up. We are at the point where it is getting harder to get issues committed. Avoid any regressions, if possible, because we cannot assume a follow up will get committed.

There is no regression. Its not follow-up that we need to create but instead completely new issue which actually is opened already #2399247: Reduce number of different font-sizes in Bartik for better consistency.

yup and as long as bartik have these font issues we "maybe" should not change markup for bartik when we fix classy & core simply cause it keeps issues stalling as happend with this cause of its use of em's - Anyways lets get this in so the work on sorting comments can get done

@mortendk: We should still fight for the same goal (making Drupal as awesome as possible) even though everyone has their own interests. Changing core/Classy markup simply cannot break Bartik or Seven. I would be more than happy to have automated tests for this so it wouldn't be even possible! In the Bartik queues the focus has been heavily focused on fixing these problems so that in future it could be a lot more stable and changes in the core/Classy could be applied easier and it would be more predictable what kind of issues changes are causing. Anyway I'd like to encourage everyone to help on those issues since they would make it easier to fix things in core/Classy.

@laurii were not making a better product, we are wasting valuable time on details that really matter, yes a huge "design" change in bartik of 3 pixels, makes no sense to stop a patch for coming in - maybe its a policy change that we need then, and thats why im raising this issue.
Blindly following pixel perfection on a theme that was never pixel perfect to begin with is stupid. If this was a client project, it would have been pushed to production 3 months ago and a followup later on would then have fixed any "scandalous" 0.01px issues.

We could be way more productive and fast instead we choose to constantly stop at every chance (spelling error in documentation a missing dot whatever else we can find, that could be simple followups) This is frustrating for everybody and in the end makes any kinda of change taking months instead of hours, thats not good for anything.

@mortendk You're not in a position to decide which details matter or not. Can you imagine all the people who have rolled their eyes when you've been making demands over an extra div or whitespace? They are not in a position to tell you that your concerns don't matter and neither are you.

You've spent more than double the time moaning and arguing then it took to actually fix the issues, and you're complaining about productivity? This does not include the time spent by others who have to deal with your outbursts.

@lewis im totally aware im not in that position ;)
That won't stop me in raising the problems for the way we are working. We are developing in a way that isnt as productive as we could be - we have turned out to be our own biggest enemy without even realising it (when anything takes 4 months for changing anything we have an issue)

My "moaning" as you flattering are calling it (...) is on the general level of how we are not moving forward in the speed we could, were hella unproductive. Yes it should be elevated to a meta instead, (cause its policy & not just code ) as this issue with 400 comments in makes alex pots head hurt when this one day hits the inbox of the committers :P

cheers

This patch only changes markup and therefore is permitted in beta. Committed d4011a5 and pushed to 8.0.x. Thanks!