Problem/Motivation

The Style for the marker of a new comment looks like the content of the comment. Needs to diminished importance and/or highlight the marker to distinguish it from the body of the comment.

comments new issue

Proposed resolution

Add CSS to style the bartik new comment marker. Use theme_mark to keep the markup consistent

Remaining tasks

Implement theme_mark in a patch
Review

User interface changes

Comment new marker style to distinguish change from body copy

API changes

n/a

CommentFileSizeAuthor
#14 new-comments-in-bartik-highlight-2277643-14.patch9.52 KBlauriii
#13 new-comments-in-bartik-highlight-2277643-13.patch6.45 KBlauriii
#11 new_indicator_red.png50.22 KBteroelonen
#11 new-comments-in-bartik-highlight-2277643-11.patch1.61 KBteroelonen
#9 new-comments-in-bartik-highlight-2277643-9.patch1.25 KBlauriii
#9 Screen Shot 2014-06-28 at 13.45.46.png36.23 KBlauriii
#4 Screen Shot 2014-06-01 at 1.56.30 PM.png206.89 KBokami
#3 new-comments-in-bartik-highlight.2277643.3.patch363 bytesnatanmoraes
Test___Site-Install.png199.88 KBjoelpittet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
Issue tags: +frontend
natanmoraes’s picture

natanmoraes
he/him
Primary language Portuguese, Brazil
CreditAttribution: natanmoraes commented
Assigned: Unassigned » natanmoraes
natanmoraes’s picture

natanmoraes
he/him
Primary language Portuguese, Brazil
CreditAttribution: natanmoraes commented
Assigned: natanmoraes » Unassigned
Status: Active » Needs review
FileSize
new-comments-in-bartik-highlight.2277643.3.patch363 bytes

Updated the "new" mark to be highlighted in red.

okami’s picture

okami CreditAttribution: okami commented
Status: Needs review » Reviewed & tested by the community
FileSize
Screen Shot 2014-06-01 at 1.56.30 PM.png206.89 KB

I applied the patch and added a comment field, then added a comment and saw the styled new text.

I think this fixes the issue.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work

Let's use theme_mark here. And then this will be consistent with how the node "new" is marked up.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
Issue summary: View changes
Issue tags: -Novice
natanmoraes’s picture

natanmoraes
he/him
Primary language Portuguese, Brazil
CreditAttribution: natanmoraes commented

What do you guys mean by "use theme_mark"? Isn't the "new" mark being rendered properly already?

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented

theme_mark() is now mark.html.twig (things can move fast around here). What @alexpott is saying is that the "new" marker for comments should use our standard mark theme hook/template rather than its own custom markup. Then likely no CSS changes would be needed.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii commented
FileSize
Screen Shot 2014-06-28 at 13.45.46.png36.23 KB
new-comments-in-bartik-highlight-2277643-9.patch1.25 KB

After changing rendering method I couldn't find any changes on the layout.

teroelonen’s picture

teroelonen CreditAttribution: teroelonen commented
Assigned: Unassigned » teroelonen
teroelonen’s picture

teroelonen CreditAttribution: teroelonen commented
Status: Needs work » Needs review
FileSize
new-comments-in-bartik-highlight-2277643-11.patch1.61 KB
new_indicator_red.png50.22 KB

Merged both patches and created an other one. Screenshot of the result:

Status: Needs review » Needs work

The last submitted patch, 11: new-comments-in-bartik-highlight-2277643-11.patch, failed testing.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii commented
Status: Needs work » Needs review
FileSize
new-comments-in-bartik-highlight-2277643-13.patch6.45 KB
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii commented
FileSize
new-comments-in-bartik-highlight-2277643-14.patch9.52 KB

This patch also removes deprecated Javascript file.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented

Quick drive by review, thanks @lauriii and @teroelonen for jumping on this one. I think the last point is the most important at this time, we need an answer there - maybe there is automated test coverage somewhere for that?

  1. +++ b/core/modules/comment/comment.module
@@ -1351,6 +1351,12 @@ function template_preprocess_comment(&$variables) {
+  $mark = array(
+    '#theme' => 'mark',
+    '#status' => comment_mark($comment->getCommentedEntityId(), $comment->getCreatedTime()),
+  );
+  $variables['new'] = drupal_render($mark);

    Try setting the variable as a render array here, there shouldn't be a need to pre-render it.

    $variables['new'] = array('#theme' => 'mark') 

  2. +++ b/core/modules/comment/comment.module
@@ -1358,6 +1364,33 @@ function template_preprocess_comment(&$variables) {
+function comment_mark($nid, $timestamp) {
+
+  $cache = &drupal_static(__FUNCTION__, array());

    Extra blank line here. 

  3. +++ b/core/modules/comment/templates/comment.html.twig
@@ -67,12 +67,7 @@
-  {#
-  Hide the "new" indicator by default, let a piece of JavaScript ask
-  the server which comments are new for the user. Rendering the final
-  "new" indicator here would break the render cache.
-  #}
-  <mark class="hidden new" data-comment-timestamp="{{ new_indicator_timestamp }}"></mark>

    Wait a minute, can we be sure this doesn't break caching? And if that's not a problem, what's happening to the new_indicator_timestamp variable?

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii commented

Actually I've been testing this manually already for a while, and because of comments are cached, so is the new marker also.

And because by manually testing this seems to be broken, I'm not sure if there's automated testing coverage for that because automated tests seems to pass.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented

Oh I guess we don't have automated testing because JavaScript. Doh.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
Status: Needs review » Needs work

Setting to needs work based on #15

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii commented
Assigned: teroelonen » Unassigned
Issue tags: +Needs issue summary update

This needs issue summary update because whats described there isn't relevant anymore. I guess we have to stick with the javascript solution to not break caching.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Version: 8.0.x-dev » 8.1.x-dev
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Related issues: +#1311372: Use <mark> element for 'mark' theme hook
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented 
+++ b/core/modules/comment/comment.module
@@ -1351,6 +1351,12 @@ function template_preprocess_comment(&$variables) {
+    '#status' => comment_mark($comment->getCommentedEntityId(), $comment->getCreatedTime()),
...
+  $variables['new'] = drupal_render($mark);

this is no go now, so JS is a way

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.