Comments

fotuzlab’s picture

Assigned: Unassigned » fotuzlab
rahul.shinde’s picture

Assigned: fotuzlab » rahul.shinde
Issue tags: +#drupalgoa2015
rahul.shinde’s picture

Status: Active » Needs review
FileSize
1.85 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,772 pass(es). View
rahul.shinde’s picture

Assigned: rahul.shinde » Unassigned
andypost’s picture

Status: Needs review » Needs work

Great! now we need to add tests

rahul.shinde’s picture

Assigned: Unassigned » rahul.shinde
rahul.shinde’s picture

Status: Needs work » Needs review
FileSize
3.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,790 pass(es). View

Uploading new patch with web-test and few changes.

rahul.shinde’s picture

Assigned: rahul.shinde » Unassigned
rahul.shinde’s picture

Assigned: Unassigned » rahul.shinde
Status: Needs review » Needs work
  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.

rahul.shinde’s picture

Assigned: rahul.shinde » Unassigned
Status: Needs work » Needs review
FileSize
1.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,795 pass(es), 1 fail(s), and 0 exception(s). View
3.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,493 pass(es). View

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 last submitted patch, 10: comment_links_weight-test-2428509-10.patch, failed testing.

andypost’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

There's test and fix

Wim Leers’s picture

Issue tags: +DrupalWTF

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  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?

rahul.shinde’s picture

@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

andypost’s picture

Also we can use #prefix/suffix or #theme_wrapper => tag

andypost’s picture

another option to use own field__comment__links template, but that needs profiling

andypost’s picture

Status: Needs work » Needs review
FileSize
723 bytes
3.43 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2428509-comment_links_weight-18.patch. Unable to apply patch. See the log in the details link for more information. View

NAV added only by bartik, so move it to preprocess

PS: theme wrappers does not work for "html_tag" because it's a render element not a theme function, #17 will not work because link is not a field

andypost’s picture

Issue tags: +Twig
emma.maria’s picture

Issue tags: +frontend

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

andypost’s picture

Will require re-roll after #2031883: Markup for: comment.html.twig

Cottser’s picture

  • +++ b/core/themes/bartik/bartik.theme
    @@ -103,6 +103,17 @@ function bartik_preprocess_block(&$variables) {
    +    // Add NAV wrapper element for comment links.
    +    $variables['content']['links']['#prefix'] = '<nav>';
    +    $variables['content']['links']['#suffix'] = '</nav>';
    

    What happens with this if there are no links?

    Would be nice to s/NAV/<nav>/ I think :)

  • Status: Needs review » Needs work

    The last submitted patch, 18: 2428509-comment_links_weight-18.patch, failed testing.

    andypost’s picture

    Status: Needs work » Needs review
    FileSize
    577 bytes
    2.39 KB
    FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,614 pass(es), 63 fail(s), and 0 exception(s). View

    re-roll and fixed comment

    @Cottser I'm sure that HTML-tags are always written in caps, see #2542392-8: Document scripts_bottom template variable

    +++ b/core/themes/bartik/bartik.theme
    @@ -103,6 +103,17 @@ function bartik_preprocess_block(&$variables) {
    +  if (isset($variables['content']['links'])) {
    

    maybe better check with Element::children()?

    andypost’s picture

    FileSize
    719 bytes
    1.69 KB
    FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,682 pass(es), 1 fail(s), and 0 exception(s). View

    I'm pretty sure that "nav" here is wring, because comment links are not a part of main navigation

    The last submitted patch, 25: 2428509-comment_links_weight-25.patch, failed testing.

    Status: Needs review » Needs work

    The last submitted patch, 26: 2428509-comment_links_weight-26.patch, failed testing.

    emma.maria’s picture

    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.

    andypost’s picture

    Status: Needs work » Needs review
    FileSize
    423 bytes
    2.1 KB
    FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,344 pass(es), 1 fail(s), and 0 exception(s). View

    Fixed 1px!
    @emma.maria awesome find!

    Status: Needs review » Needs work

    The last submitted patch, 30: 2428509-comment_links_weight-30.patch, failed testing.

    andypost’s picture

    somehow no comment body rendered...

    The last submitted patch, 30: 2428509-comment_links_weight-30.patch, failed testing.

    andypost’s picture

    Status: Needs work » Needs review
    FileSize
    1.86 KB
    2.77 KB
    PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,711 pass(es). View

    Fixed tests, looks iterate through elements is only way

    larowlan’s picture

    Assigned: Unassigned » emma.maria
    +++ 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

    andypost’s picture

    there's a meta/initiative to remove uses of random in tests to prevent random failsthere's a meta/initiative to remove uses of random in tests to prevent random fails

    I'm pretty sure that this one should use randomString() to generate comment body because comment body contains user input that is not "machine name"

    andypost’s picture

    is it possible to get this in before release?

    Truptti’s picture

    FileSize
    96.77 KB

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

    andypost’s picture

    FileSize
    2.76 KB

    re-roll

    emma.maria’s picture

    Assigned: emma.maria » Unassigned
    Issue summary: View changes
    Status: Needs review » Reviewed & tested by the community
    FileSize
    73.43 KB
    54.53 KB

    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!
     

     

     

    Cottser’s picture

    Version: 8.0.x-dev » 8.1.x-dev
    Status: Reviewed & tested by the community » Needs review

    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?

    andypost’s picture

    Yep, test should switch theme to Bartik
    Maybe better to extract test to separate method with Bartik theme enabled?
    @larowlan any ideas?

    andypost’s picture

    Version: 8.1.x-dev » 8.2.x-dev
    Status: Needs review » Reviewed & tested by the community

    Suppose it not too late for d8

    alexpott’s picture

    Status: Reviewed & tested by the community » Fixed

    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!

    • alexpott committed 89df46f on 8.3.x
      Issue #2428509 by andypost, rahul.shinde, emma.maria: Comment links...

    • alexpott committed 3cd6347 on 8.2.x
      Issue #2428509 by andypost, rahul.shinde, emma.maria: Comment links...
    andypost’s picture

    Status: Fixed » Closed (fixed)

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

    lauriii’s picture

    Component: comment.module » Bartik theme

    Just moving to the right queue