Suggested commit message :

git commit -m 'Issue #2581407 by davidhernandez, LewisNyman, emma.maria, Saphyel: Remove the obsoletely named Classy library attached to Bartik'\''s comment.html.twig as Bartik uses its own styles. '

Problem/Motivation

Comments that were replies to other comments used to be wrapped in the indented class, I traced this back to working in beta 14. Then something happened.

Beta 14.

 

 

HEAD right now.

 

 
This has a big impact on Bartik as it styles a reply by indenting it to sit under the comment it is replying to.

Proposed resolution

If this feature was removed for a reason (which I can't find anywhere) I think themes still need a class in the markup so replies can have targeted styles added to them.

Remaining tasks

Figure out why this has happened

Fix it with a patch :-)

User interface changes

Put back the indented comment visual feature.

API changes

none

Data model changes

none

CommentFileSizeAuthor
#21 Screenshot from 2016-03-05 12-26-00.png52.56 KBSaphyel
#17 remove_the_classy-2581407-17.patch440 bytesdavidhernandez
#11 regression_comments-2581407-10.patch428 bytesLewisNyman
#2 commentindented.png206.15 KBdavidhernandez
head-screenshot.png65.08 KBemma.maria
head-markup.png75.36 KBemma.maria
beta-14-indented-markup.png65.13 KBemma.maria
beta-14-screenshot.png50.92 KBemma.maria
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

emma.maria created an issue. See original summary.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez as a volunteer commented
Title: Comments replying to comments have lost their markup to differentiate them from direct comments on nodes. » REGRESSION: Comments replying to comments have lost their markup to differentiate them from direct comments on nodes.
FileSize
commentindented.png206.15 KB

Not sure what is up with the markup. I'm on head:

commit 4811558a8514888dafeea1b9ff55796bb21b17ca
Author: xjm <xjm@65776.no-reply.drupal.org>
Date:   Tue Oct 6 16:30:40 2015 -0500

and I see the correct markup. See screenshot. The CSS however needs to be fixed. The indented comment is a sibling of the comment replied to, so it needs sibling selector. not sure when this template/logic changed.

I don't know why Emma and I are seeing different markup.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid as a volunteer commented

I can confirm I'm seeing the same broken display as emma.maria.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid as a volunteer commented
Component: Classy theme » Bartik theme
davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez as a volunteer commented

Just to double check, were replies actually made, and not just relying on devel to generate comments? Maybe the generator isn't replying properly.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid as a volunteer commented

Yes, I made the replies entirely through the UI, and not using devel generated content.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez as a volunteer commented

I'm still seeing the correct markup, but I did find one spot of regression. The Classy library name was changed to "indented". That needs to be updated in Bartik's comment template and why the CSS for Classy isn't loading.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Title: REGRESSION: Comments replying to comments have lost their markup to differentiate them from direct comments on nodes. » REGRESSION: Comments replying to comments have lost their styles to differentiate them from direct comments on nodes.
Component: Bartik theme » Classy theme

So I tested again to triple check and indented markup is printing. I think I messed up my 8.0.x branch testing.

The styling of indented comments CSS is already broken for Bartik as pointed out here in this Bartik issue #2512468: Regression: Indented styles for indented comments are missing in Bartik.. So it turns out that the indented.css file from Classy with working indented styles is now currently not being loaded on the frontend. That is the actual bug here.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Issue summary: View changes
emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented

On IRC @davidhernandez has pointed out:

so Bartik has it's own comment template. The classy library is loaded there, but we changed the name of the library. that is the regression. the name change.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Component: Classy theme » Bartik theme
Status: Active » Needs review
FileSize
regression_comments-2581407-10.patch428 bytes

Looks like we forgot to update the library name in the Bartik template. Here's the fix.

Status: Needs review » Needs work

The last submitted patch, 11: regression_comments-2581407-10.patch, failed testing.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez as a volunteer commented

That "indented" library from Classy is only adding two rules used for indenting the threaded comments. That is something Bartik is trying to supply itself, so I'm thinking just delete the library attaching in Bartik's comment template and fix the CSS in the other issue. This doesn't need to postpone the other one.

#2512468: Regression: Indented styles for indented comments are missing in Bartik.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez as a volunteer commented

That could also be deleted in the other issue.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Title: REGRESSION: Comments replying to comments have lost their styles to differentiate them from direct comments on nodes. » Remove the Classy library attached to Bartik's comment.html.twig as Bartik uses it's own styles.
Version: 8.0.x-dev » 8.1.x-dev
Category: Bug report » Task
Issue tags: -Usability +Novice, +Needs manual testing

Refactoring this issue as Bartik itself fixed the loss of indentation in #2512468: Regression: Indented styles for indented comments are missing in Bartik. using it's own CSS. But we still need to carry out the task of removing the attached Classy library as mentioned by David in #13 as Bartik provides all of it's own style for comments.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria at Deeson commented
davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez at FFW commented
Status: Needs work » Needs review
FileSize
remove_the_classy-2581407-17.patch440 bytes

Removing the library from the template.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Title: Remove the Classy library attached to Bartik's comment.html.twig as Bartik uses it's own styles. » Remove the obsoletely named Classy library attached to Bartik's comment.html.twig as Bartik uses its own styles.

Ran into this bug today, updating the description slightly.

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.

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Issue tags: +Needs screenshots

To complete this issue we need someone to test the patch in #17 check that comments in Bartik look the same Before and After and post screenshots. Then this issue will be RTBC.

Saphyel’s picture

Saphyel CreditAttribution: Saphyel as a volunteer commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +dclondon
FileSize
Screenshot from 2016-03-05 12-26-00.png52.56 KB

Review and now works as Beta 14

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Issue summary: View changes

Suggested commit message :

git commit -m 'Issue #2581407 by davidhernandez, LewisNyman, emma.maria, Saphyel: Remove the obsoletely named Classy library attached to Bartik'\''s comment.html.twig as Bartik uses its own styles. '

  • catch committed 47706f7 on 8.2.x 
    Issue #2581407 by davidhernandez, LewisNyman, emma.maria, Saphyel:...

  • catch committed 2cf0381 on 8.1.x 
    Issue #2581407 by davidhernandez, LewisNyman, emma.maria, Saphyel:...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 8.2.x-dev » 8.1.x-dev
Category: Task » Bug report
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.2.x and cherry-picked to 8.1.x, thanks!

Status: Fixed » Closed (fixed)

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