Clean up the "comments" component in Bartik

Ok, spend some time on testing comments and looking the code.

1. Comment arrow is currently broken. Before starting to actually work with this I fixed it to #2398531: Some images not showing up after SMACSS split
2. Unpublished content is kinda impossible to notice as Bartik current styles are overriding cores unpublished styles. We're adding a tiny pink borders to comment arrow when unpublished (see screenshot), but it took me like few minutes to notice there's actually something. Related to this theres few issues around on this f.ex on #1854376: Bartik overrides unpublished style from node.css

On 2. I'd rather add the pink background.

The code for unpublished comments also exists both in comments.css and content.css. I think on this issue we're not going to change content.css, right?

And a patch.

Don't consider this to be ready. Just sending it to testbot.

Setting back to 'Needs work' per #4

Don't consider this to be ready. Just sending it to testbot.

+++ b/core/themes/bartik/css/components/comments.css
@@ -1,4 +1,8 @@
+ *

I don't think there should be empty row

Lets take a new start here.

1. .comment h2.title removed as there's no h2 in .comment. There's h3 in .comment and h2 in .comment-wrapper
2. @lewisnyman pointed out that we're not making visual changes on these, so I'll revert the mention about unpublished colors.
3. Made some changes to template.
4. Made some changes to css-file based on changes on template
5. The missing arrow bug was already fixed on other issue.

A patch as attachment. Don't think it's ready, but some feedback would be great.

patch for comment.css there is an extra space .comment-footer { display: table-row; } and added outline: none in comment subject field

Setting to needs review so the testbot triggers

Worked a bit on this. Didn't yet fix all things mentioned by emmamaria.

You can forget the one in #21. Something went wrong.

I guess we still have to print the parent item and print content attributes. Maybe we just remove the class as Emma suggested?

Reviewing...

Looking back at #20 Emma's list, I've found a couple of things still need updating, so here's an update that I believe includes all 4 requirements at #20.

1. +++ b/core/themes/bartik/css/components/comments.css
   @@ -1,7 +1,12 @@
   -.comment h2.title {
   -  margin-bottom: 1em;
   +.comment {
   +  margin-bottom: 20px;
   +  display: table;
   +  vertical-align: top;
   

   The .comment styling is now added twice in the stylesheet

2. +++ b/core/themes/bartik/css/components/comments.css
   @@ -1,7 +1,12 @@
    .comment .field-name-field-user-picture img {
      margin-left: 0; /* LTR */
   

   An <img/> element has 0 margin by default. This code can be removed

3. +++ b/core/themes/bartik/css/components/comments.css
   @@ -14,40 +19,40 @@
   +.comment__attribution img {
      margin: 0;
   

   Same here

4. +++ b/core/themes/bartik/css/components/comments.css
   @@ -14,40 +19,40 @@
   -.comment .comment-arrow {
   +.comment-arrow {
   

   This should be .comment__arrow since it is nested in .comment

5. +++ b/core/themes/bartik/css/components/comments.css
   @@ -57,12 +62,12 @@
   +.comment-text {
   

   This should be .comment__text since it is nested in the .comment component

6. A comment ends with the .comment-footer class. This should be .comment__footer since it is nested in the .comment component.

I've incorporated all of idebr's comments at #26 and also noted that the comment-arrow border colours that used to be defined in colors.css are (rightly) not there any more - so I've added them here. I *believe* that falls within the scope of this issue - I'm happy to create a new one if required...

Also note that I'm unsure about renaming some of the classes still using old comment-xxx structure in there. I *think* they should be converted but want to check...

1. +++ b/core/themes/bartik/css/components/comments.css
   @@ -1,68 +1,62 @@
   + * Visual styles for comments in Bartik
   

   Comments should end in a full stop.

2. +++ b/core/themes/bartik/css/components/comments.css
   @@ -1,68 +1,62 @@
   -.comment h2.title {
   -  margin-bottom: 1em;
   -}
   

   This <h2> used to belong to the comment-wrapper. If we remove it here, we should at least open a followup to reapply it to the comment-wrapper

3. +++ b/core/themes/bartik/css/components/comments.css
   @@ -1,68 +1,62 @@
   -.comment .attribution .username {
   +.comment__author {
      white-space: nowrap;
    }
   

   This breaks the username display, since the attribute now applies to the wrapper instead of the child element.

4. +++ b/core/themes/bartik/css/components/comments.css
   @@ -1,68 +1,62 @@
   -.comment .submitted .comment-time {
   +.comment__created {
   

   This selector should be .comment__time per #20.3

5. +++ b/core/themes/bartik/css/components/comments.css
   @@ -1,68 +1,62 @@
   +  border-color: #ffffff;
   

   Use the shorthand color code when possible, eg. #fff. If you copied the selector from colors.css, please remove it from the original file.

6. +++ b/core/themes/bartik/css/components/comments.css
   @@ -1,68 +1,62 @@
   -.comment .comment-text {
   +.comment-text {
   

   This should be .comment__text since it is a child element of .comment

7. +++ b/core/themes/bartik/css/components/comments.css
   @@ -95,11 +89,10 @@
   +.comment.unpublished .comment__arrow {
   

   unpublished is a modifier, so the selector should be .comment--unpublished

8. +++ b/core/themes/bartik/css/components/comments.css
   @@ -95,11 +89,10 @@
    .comment-footer {
      display: table-row;
    }
   

   The markup should be .comment__footer since it is a child element of .comment. The css does not appear to do anything, since its parent is not a table. If so, it can be removed.

Some other points that are not in the patch:

1. typography.css contains a selector p.comment-time that should be updated with the new markup. The selector can be moved to comments.css optionally in the same way #2398447: Remove the "typography" CSS file in Bartik does.
2. content.css contains this selector that should be moved and updated:

   .node--unpublished .comment-text .comment-arrow,
   .unpublished .comment-text .comment-arrow {
     border-left: 1px solid #fff4f4;
     border-right: 1px solid #fff4f4;
   }

I'm going to look at this today after around 2.30-3pm UTC unless anyone else wants to work on it.

The patch in #29 is out of date, so I've re-rolled it with a slight tweak ("commment__content") and attached. I've interdiffed manually and it looks right. I think it's the font-family statement that was preventing #29 applying.

I'll now look at @idebr's feedback - thanks for that! If I don't get that done by 1700UTC then I'll be at a meeting so I'll try to remember to unassign it!

Thanks @jp.stacey! There's no rush, so feel free to work on it at your own pace.

Please find a patch attached, plus interdiff. I think this covers all feedback from @idebr in #31, minus their original point 2, but plus the second extra point mentioned. Outstanding points are therefore:

* **Feedback #2: This h2 used to belong to the comment-wrapper. If we remove it here, we should at least open a followup to reapply it to the comment-wrapper**: I didn't want to make the call for how to open the followup, what parent issue to assign it to etc.
* **Extra point #1: typography.css contains a selector p.comment-time that should be updated with the new markup.** As above, I think this might be best suited to opening a followup, to fix once this and #2398447: Remove the "typography" CSS file in Bartik are both closed. Otherwise the overlap means we might get into a race condition with that other ticket!

As an addendum, unfortunately I've also just hosed my local D8 build, just as I was testing my last bit of code: changing of comment selector from .comment.unpublished to comment--unpublished (so using Twig string concatenators.) I believe what's in the patch is syntactically correct but I would appreciate other eyes specifically on that bit.

(Oh, weirdly #2398447: Remove the "typography" CSS file in Bartik is showing as crossed out in my new comment, but not in @idebr's. So possibly that work can be done here.)

Thanks for working on this, jp.stacey! I have done another review and found a few more things before this can be commited.

1. +++ b/core/themes/bartik/css/components/comments.css
   @@ -1,69 +1,63 @@
   -.comment h2.title {
   -  margin-bottom: 1em;
   -}
   

   Let's reintroduce this selector as .comment-wrapper h2 to prevent this style from being lost.

2. +++ b/core/themes/bartik/css/components/comments.css
   @@ -1,69 +1,63 @@
   -.comment .attribution img {
   -  margin: 0;
   

   This selector is necessary to win specificity over .field-type-image img. Let's introduce a new selector .comment .field-type-image img with just the margin: 0 declaration as a comment to indicate the selector is overqualified to win specificity over .field-type-image img

3. +++ b/core/themes/bartik/css/components/comments.css
   @@ -87,20 +81,20 @@
    }
   -.comment.unpublished .comment-text .comment-arrow {
   +.node--unpublished .comment__text .comment__arrow,
   +.comment--unpublished .comment__text .comment__arrow {
   

   The border color overrides are used to fix the white borders when a node is unpublished and gets a light red background-color. An unpublished comment has no background-color, so .comment--unpublished .comment__text .comment__arrow can be removed. The selector is overqualified and can be rewritten as .node--unpublished .comment__arrow

4. +++ b/core/themes/bartik/css/components/comments.css
   @@ -87,20 +81,20 @@
    .comment-footer {
      display: table-row;
    }
   

   This selector no longer applies since the class has been changed to comment__footer in the html markup. Since the parent is not displayed as a table, this selector can probably be removed. Please test manually to confirm.

Thanks, @idebr. Looking at this now.

Attached patch and interdiff.

@idebr, referring to your most recent round of changes:

1. #content h2 in content.css takes precedence unless this selector is #content .comment-wrapper h2 so I've used that.
2. I've copied this verbatim as you requested, but this would affect all fields of type image on comments, not just attribution images. Do we care?
3. Done
4. Done: looks fine here in Chrome. As you say I can't see display: table-row doing much inside non-table elements.

I've yet to open the two followup tickets I detailed in #36. Anyone willing to take responsibility for their correct metadata? :)

This is looking really good, thanks jp.stacey :)

1. 1. #content h2 in content.css takes precedence unless this selector is #content .comment-wrapper h2 so I've used that.

   Excellent! In a case like this where we use an overqualified selector to override styling from other selectors, please add a comment why the selector is overqualified. For an example, have a look at the Seven theme: http://cgit.drupalcode.org/drupal/tree/core/themes/seven/css/components/...

   /* This is required to win over specificity of [dir="rtl"] .tabs__tab */

2. 2. I've copied this verbatim as you requested, but this would affect all fields of type image on comments, not just attribution images. Do we care?

   That's a good point. Let's not add any more overrides then necessary and switch the selector to .comment .field-name-user-picture img. This selector is also overqualified, so it needs a similar comment as above to make clear the selector is overqualified to override .field-type-image img.

Regarding the follow-ups:

• The css for the comment wrapper title has now been in this issue, so that is already covered.
• The issue #2398447: Remove the "typography" CSS file in Bartik has already been committed and its changed have successfully been merged into the most recent patch, so that is covered as well

OK, attached is a file and interdiff with the most recent round of changes for review.

Thanks @jp.stacey! I did a manual test as well to confirm nothing broke. Screenshots here:

Before/after:

Run some tests with BackstopJS and everything looks good. RTBC++

What does an unpublished comment look like now?

+++ b/core/themes/bartik/css/components/comments.css
@@ -87,20 +89,16 @@
-.comment.unpublished {
+.comment--unpublished {
   margin-right: 5px; /* LTR */
   padding: 5px 2px 5px 5px; /* LTR */
 }
-[dir="rtl"] .comment.unpublished {
+[dir="rtl"] .comment--unpublished {
   margin-left: 5px;
   margin-right: 0;
   padding: 5px 5px 5px 2px;
 }
-.comment.unpublished .comment-text .comment-arrow {
+.node--unpublished .comment__arrow {
   border-left: 1px solid #fff4f4;
   border-right: 1px solid #fff4f4;
 }

+++ b/core/themes/bartik/css/components/content.css
@@ -123,11 +123,6 @@ ul.links {
-.node--unpublished .comment-text .comment-arrow,
-.unpublished .comment-text .comment-arrow {
-  border-left: 1px solid #fff4f4;
-  border-right: 1px solid #fff4f4;
-}

What does an unpublished comment on a published node and on an unpublished node look like? And what does a published comment on an unpublished node look like?

Good catch.

@DickJohnson The border colors are to blend in with unpublished nodes that have a light red background. Unpublished comments have no background-color, as you noticed yourself in #2422113: Unpublished comments have lost their styles in D8

I still don't get what I'm missing. After my latest patch unpublished comments look like:

And unpublished nodes have pink background.

Looks like your browser has zoomed out

I replaced the comment arrow with a css triangle. The technique is explained on CSS tricks: http://css-tricks.com/snippets/css/css-triangle

The patch is based on #43, since the interdiff in #47 suggests an oversight by DickJohnson on the .unpublished selector

@idebr that's a neat drop-in and good to get rid of the image.

I've reviewed the patch in #52 and it applies cleanly. I've also reviewed published and unpublished nodes' comment threads, before and after the patch is applied. They look great to me and the only difference seems to be the welcome return of the margin under h2.title.

However, I can't for the life of me detect the nuances mentioned by @alexpott between the 2^3=8 different states: patched/unpatched; node published/unpublished; comment approved/unapproved. The only obvious difference to me for unapproved comments is they get a right-hand margin (left-hand in RTL).

I've therefore uploaded screenshots of all states (4, not 8, but the thread always contains a comment, a reply and an unapproved) so maybe if there are still changes on this ticket, we can discuss with reference to these.

Patch no longer applies after #2422113: Unpublished comments have lost their styles in D8 was committed. I'll work on a reroll.

Rerolled the patch and applied the unpublished styling that was already available for nodes to comments as well.

Reviewed +1 here: patch applies cleanly, visuals look exactly the same to me as with patch from #52!

The interdiff on #55 didn't take into account the rerolling owing to #2422113: Unpublished comments have lost their styles in D8, so I attach another for reference which is literally every line change: a bit noisier that way, but it might be of use.

It'd be great to get another review of this soon, so we don't have to keep rerolling to hit the moving target of HEAD.

Patch applies to latest head,
code looks good,
loving the inline css triangle,
screenshot shows no noticeable visual changes.

I'd say it's a good patch! :-)

Sorry, this dropped off my radar. Changes look good, are isolated to Bartik, and testing looks nice and thorough. Thanks!

Committed and pushed to 8.0.x.