Comment links weight should be managed by view display

Uploading new patch with web-test and few changes.

1. +++ b/core/modules/comment/src/Tests/CommentLinksTest.php
   @@ -104,6 +104,18 @@ public function testCommentLinks() {
   +    $this->assertTrue($this->cssSelect('article > div ul:first-child'), 'Links found before comment body.');
   

   We need to call node page to check functionality. Input from @andypost

2. +++ b/core/modules/comment/src/Tests/CommentLinksTest.php
   @@ -104,6 +104,18 @@ public function testCommentLinks() {
   +    $this->assertTrue($this->cssSelect('article > div ul:last-child'), 'Links found after comment body.');
   

   Same goes here too.

Adding two patches,

1. Patch with template changes and webtest comment_links_weight-2428509-10.patch
2. Webtest only path is comment_links_weight-test-2428509-10.patch

The "Links" component is also present in HEAD, but doesn't have any effect because the templates override this. So +1 to this!

+++ b/core/themes/bartik/templates/comment.html.twig
@@ -105,15 +105,12 @@
-        <nav>{{ content.links }}</nav>

Doesn't this mean that now links won't be using <nav>?

1. +++ b/core/themes/bartik/templates/comment.html.twig
   @@ -105,15 +105,12 @@
   -        <nav>{{ content.links }}</nav>
   

   What about this <nav> tags?

2. +++ b/core/themes/classy/templates/content/comment.html.twig
   @@ -110,7 +110,7 @@
        {% if signature %}
          <div class="user-signature">
   @@ -118,7 +118,4 @@
   
   @@ -118,7 +118,4 @@
          </div>
        {% endif %}
   ...
   -  {% if content.links %}
   -    {{ content.links }}
   -  {% endif %}
   

   So links will now always come above the signature?

@alexpott,

For 1, Can we write things in Preprocessor, which is obviously not so cool thing to do.
For 2. There is a ticket saying remove signature and make it as contrib, see #1548204: Remove user signature and move it to contrib

This issues addresses the default themes so I'm adding the frontend tag for more exposure.

Thanks @andypost for pointing this out.

Yes nav should be used for major navigation and here it's being applied to an action list for each comment. I am happy to remove it I agree it's overkill, especially if we have to also add a preprocess to Bartik to support it.

For CSS purposes the <nav> wrapper is only serving the purpose of providing a 1px padding which was added incorrectly in #2031883: Markup for: comment.html.twig. I am happy to remove the nav wrapper and convert the padding to a line height on the links list.

I can add the fix for the v slight visual issue removing the nav causes using the patch in #26.

+++ b/core/modules/comment/src/Tests/CommentLinksTest.php
@@ -70,7 +70,7 @@ public function testCommentLinks() {
+      'comment_body' => array(array('value' => $this->randomMachineName())),

if you re-roll, and while we're touching this - can re remove the reliance on random here (there's a meta/initiative to remove uses of random in tests to prevent random fails)

Other than that +1 RTBC - but leaving for Emma as this touches Bartik too - assigning accordingly

Got error on applying patch in #34,attaching snapshot for reference.

Argh sorry for holding this up! I'm happy with everything Bartik in this issue.

Also for good measure, I tested it out and links can now print above the comment. Woop!
 

 

 

Thanks everyone.

It's too late for 8.0.x for this one, bumping to 8.1.x for now. I'm also not sure what the test here is really doing, the test doesn't seem to be testing Bartik (it's testing Stark) and Bartik is the only functional (non-test) code we're changing. Am I missing something? I'm not against adding test coverage but if there's similar coverage elsewhere then I don't think we should add it on account of this issue. I ran the test locally and it passes both with and without the functional part of the patch.

Should the component be Bartik for this?

Given this was pushed back from 8.0.x to 8.1.x and forgotten about and has the relevant sign off from the sub system maintainers I'm going to commit this to 8.2.x in time for 8.2.0.

Committed and pushed 89df46f to 8.3.x and 3cd6347 to 8.2.x. Thanks!

Just moving to the right queue