Problem/Motivation

Bartik's indented comment styles are incorrect and comments are currently not indented in HEAD right now.

Proposed resolution

Add the correct CSS selector and bring indented comments back.

Remaining tasks

User interface changes

Before the patch:

After the patch:

API changes

Data model changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because indented comments do not have any styling currently
Issue priority Major because reply comments need to be indented to indicate that they belong to another comment.
Unfrozen changes Unfrozen because it only changes CSS
Prioritized changes The main goal of this issue is usability
Disruption Non disruptive.
CommentFileSizeAuthor
#61 rtl.png70.05 KBemma.maria
#61 ltr.png73.75 KBemma.maria
#59 indented_styles_for-2512468-59.patch490 bytesbendev
#57 indented_styles_for-2512468-57.patch614 bytesbendev
#52 Screen-Shot-2015-10-06-at-21.38.55.png182.41 KBemma.maria
#51 Patch Applied-Commenting Issue.png69.23 KBmeenakshi.r
#51 Without Patch-Commenting Issue.png73.19 KBmeenakshi.r
#48 2512468-48.patch595 bytesbendev
#39 Issue_2512468_RTL_Comment-after-patch-20.png22.87 KBchipway
#39 Issue_2512468_RTL_Comment-before-patch-20.png26.44 KBchipway
#38 Issue_2512468_RTL_Comment-after-patch-20.png39.73 KBchipway
#38 Issue_2512468_RTL_Comment-before-patch-20.png43.67 KBchipway
#36 Issue_2512468_RTL_Comment-after-patch-20.png43.67 KBchipway
#36 Issue_2512468_LTR_Comment-after-patch-20.png45.09 KBchipway
#36 Issue_2512468_RTL_Comment-before-patch-20.png39.73 KBchipway
#36 Issue_2512468_LTR_Comment-before-patch-20.png43.27 KBchipway
#30 bartik_comments_before.png40.6 KBelchiconube
#30 bartik_comments_after.png52.11 KBelchiconube
#30 bartik_comments_after-responsive.png50.64 KBelchiconube
#26 Bartik-Before-2512468-20.png42.42 KBjim005
#26 Bartik-After-2512468-20.png41.94 KBjim005
#23 comments_seven_OK.png10.89 KBfil00dl
#23 comments_bartik_OK.png14.87 KBfil00dl
#20 2512468-20.patch637 bytesrudraram
#9 after-bartik.png234.13 KBManjit.Singh
#9 after-seven.png168.6 KBManjit.Singh
#5 comment-indented-rtl.png40.89 KBtrevorkjorlien
#5 comment-indented-ltr.png41.49 KBtrevorkjorlien
#4 2512468-4.patch557 bytesmunzirtaha
#3 indented-rtl-afterpatch.png29.8 KBcchanana
#3 indented-afterpatch.png29.66 KBcchanana
#3 indented-beforepatch.png30.13 KBcchanana
#1 2512468-1.patch441 bytesmunzirtaha
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

munzirtaha’s picture

Assigned: munzirtaha » Unassigned
Status: Active » Needs review
FileSize
441 bytes

Attached is a patch to clean it up.

cchanana’s picture

Assigned: Unassigned » cchanana
cchanana’s picture

Assigned: cchanana » Unassigned
Status: Needs review » Needs work
FileSize
30.13 KB
29.66 KB
29.8 KB

@munzirtaha : If we apply this patch then, there will be indentation on both sides which makes UI inappropriate as indentation should be from one side.

munzirtaha’s picture

Title: Merge LTR and RTL margins for indented comment » Clean up margins and paddings rules for indented comments
Component: Classy theme » Bartik theme
Issue summary: View changes
Status: Needs work » Needs review
FileSize
557 bytes

@cchanana: Thanks for your review. When I made the patch I just tested it with classy and didn't know that bartik is using classy as a base theme. In this case, bartik is the one that needs clean up. Attached is another patch to remove redundant rules from bartik.

trevorkjorlien’s picture

Applied the patch in #4 and comments appear indented correctly in both LTR and RTL.

trevorkjorlien’s picture

Status: Needs review » Reviewed & tested by the community
trevorkjorlien’s picture

Issue tags: +#DrupalNorth
Anonymous’s picture

Issue tags: +DrupalNorth2015

Updating the issue tag to include a hashless DrupalNorth2015 on behalf of the Drupal North sprinting group.

Manjit.Singh’s picture

FileSize
168.6 KB
234.13 KB

@munzirtaha thanks for contribution.

Comments are now properly indented. No bartik and seven's css code is there for indentation, All are managed through classy itself. There is minor css issue for inline links (delete, edit, reply) for RTL. Is there any separate issue for that or We have to do it in this issue. I am attaching png's, Please check spacing issue in that.

Manjit.Singh’s picture

Status: Reviewed & tested by the community » Needs work
mparker17’s picture

Issue tags: -#DrupalNorth

Removing old hashed tag.

munzirtaha’s picture

Status: Needs work » Reviewed & tested by the community

@Manjit.Singh: the wrong padding is a completely separate issues that's not related to indented comments. I just provided a patch for it in issue #2514734: A forgotten padding-right for a ul made inline links look wrong in RTL. Please, review.

LewisNyman’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs screenshots

@munzirtaha Maybe you did this by mistake but please do not set your own patch to Reviewed & tested by the community. We don't have any screenshots of the site in LTR and we don't have any screenshots of how it looked before to compare. Let's get these first before we site it to RTBC.

munzirtaha’s picture

Status: Needs review » Needs work

@LewisNyman: It's not me who set it to RTBC in the first place. It's "trevorkjorlien" who have reviewed and set it and attached a screenshot that showed the situation in both ltr and rtl. The screenshot shows the situation before and after there is no difference whatsoever. When "Manjit.Singh" set it to needs work pointing to a different bug I just explained that to him and returned it back to its previous situation. If this is still wrong behavior, I am sorry.

Manjit.Singh’s picture

@lewis, @munzirtaha had create a new issue #2514734: A forgotten padding-right for a ul made inline links look wrong in RTL .
can we create a new issue for inline links ? OR solve that in this issue itself ?

What should be best approach ?

Manjit.Singh’s picture

Status: Needs work » Needs review

setting back to needs review as we need a screenshots for intended comments.

emma.maria’s picture

Status: Needs review » Needs work

I cannot see why we are removing Bartik styles which are different to Classy.

Bartik sets 40px on the left in it's styles.

.comment .indented {
  margin-left: 40px; /* LTR */
}

But due to markup changes to comments, the current selector setting the 40px is not correct.

Can we instead fix the selector and get the Bartik styles working again please.

rudraram’s picture

Patch in #4 is getting rejected.

emma.maria’s picture

Issue tags: +Needs reroll

The patch in #4 needs a reroll plus the work added to it mentioned in #17.

rudraram’s picture

@emma.maria Updated the patch as per your suggestion in #17 and styles are getting applied from the Bartik's comments.css file.

rudraram’s picture

Status: Needs work » Needs review
Manjit.Singh’s picture

Status: Needs review » Needs work

Need some screenshots after #20.

fil00dl’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs screenshots
FileSize
14.87 KB
10.89 KB

this issue seems fixed (please find screenshots in attachment)

nicolas@webstanz.be’s picture

Issue summary: View changes

We are working on in Barcelona with joelpitter & chipway

jim005’s picture

Issue summary: View changes
jim005’s picture

chipway’s picture

Worked on the issue at Barcelona 2015 with jim005, JBO, rbrissaud,. It looks RTBC.

jim005’s picture

Status: Needs review » Reviewed & tested by the community

Tested in Barcelona DrupalCon 2015, added screenshots (before / after) patch in summary section and in comment.

rbrissaud’s picture

Worked on the issue at Barcelona 2015 with jim005, JBO, chipway. It looks RTBC.

elchiconube’s picture

It's woking well after patch here in DrupalCon Barcelona

davidhernandez’s picture

Considering this also changes RTL styles can we get right to left screenshots to verify it is ok?

davidhernandez’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 20: 2512468-20.patch, failed testing.

chipway queued 20: 2512468-20.patch for re-testing.

chipway’s picture

Working on followup of my mentorees work at Barcelona 2015:
Task: Printing screenshots for LTR and RTL languages.
2 tests related to Amazon didn't pass anymore on patch, so I asked for retest on 2512468-20.patch

While waiting for test results, I applied the patch locally with no error.
It seems to solve when we add response to an existing comment.

chipway’s picture

Issue summary: View changes
chipway’s picture

chipway’s picture

chipway’s picture

Issue summary: View changes
chipway’s picture

As last test passed (https://qa.drupal.org/pifr/test/1139948) the patch looks ready to go RTBC.

chipway’s picture

Status: Needs work » Needs review
Issue tags: +Barcelona 2015

Who could RTBC https://www.drupal.org/node/2512468#comment-10384197 @ced_cht @eboux @nichautenauve @gaelgosset @Gromit06 @WebSenso @benoitdevos @Creatile #drupalconeur

bendev’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

patch tested
indentation ok

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 20: 2512468-20.patch, failed testing.

jim005 queued 20: 2512468-20.patch for re-testing.

chipway’s picture

Status: Needs work » Reviewed & tested by the community

Last test passed with 0 failure.
(Previous fails seemed to be because of Amazon services being unavailable)

emma.maria’s picture

Status: Reviewed & tested by the community » Needs work

Thanks all for the testing of the patch in #20. However I noticed something with the code. Bartik should ideally match the CSS selector that Classy uses, see below.

From indented.css within Classy.

/**
 * Indent threaded comments.
 */
.indented {
  margin-left: 25px; /* LTR */
}
[dir="rtl"] .indented {
  margin-left: 0;
  margin-right: 25px;
}

There should be no visual changes from applying this change.

bendev’s picture

CSS selector updated according to #47 request

emma.maria’s picture

Status: Needs work » Needs review
Manjit.Singh’s picture

Issue tags: +Needs manual testing

We need a manual testing after applying the changes of #47.

meenakshi.r’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing
FileSize
73.19 KB
69.23 KB

Patch in #48 looks good.

Before applying patch.


After applying patch.

emma.maria’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Postponed
FileSize
182.41 KB

I tested the patch on the 8.0.x branch. I noticed that with and without the patch applied that reply comments are not indenting anymore and that the markup has changed at HEAD. Reply comments to comments are now not printing with the indented class wrapped around them to differentiate them with 1st level comments.
 

 
Postponing this issue and raising a major bug issue for the above.

emma.maria’s picture

bendev’s picture

@emma.maria are you sure ? I tested it again tonight and it still worked with the patch...

emma.maria’s picture

Priority: Normal » Major
Status: Postponed » Needs work

OK after some detective work and thinking in the other issue, this issue can be worked on. Bumping up to major because at HEAD in Bartik right now reply comments do not have any differentiating styles.

Setting to Needs Work as the RTL styles are missing the margin-left reset on them...

+++ b/core/themes/bartik/css/components/comments.css
@@ -108,15 +108,11 @@
+.indented {
   margin-left: 40px; /* LTR */
 }
-[dir="rtl"] .comment .indented {
+[dir="rtl"] .indented {
   margin-right: 40px;
-  margin-left: 0;
-}

Please put back the margin-left: 0 on the RTL styles to cancel out the LTR styles.

emma.maria’s picture

Title: Clean up margins and paddings rules for indented comments » Indented styles for indented comments are missing in Bartik.
bendev’s picture

Status: Needs work » Needs review
FileSize
614 bytes

here it is

emma.maria’s picture

Status: Needs review » Needs work
+++ b/core/themes/bartik/css/components/comments.css
@@ -108,15 +108,12 @@
-  margin-left: 0;
-}
-.comment .links {
-  padding: 0 0 0.25em 0;
+  maring-left: 0;
 }

Oops there is a typo! Also can we put back the .comment .links { code back in also. That has nothing to do with this issue.

bendev’s picture

sorry for the typo

bendev’s picture

Status: Needs work » Needs review
emma.maria’s picture

Title: Indented styles for indented comments are missing in Bartik. » Regression: Indented styles for indented comments are missing in Bartik.
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs beta evaluation
FileSize
73.75 KB
70.05 KB

Perfect! Thank you @bendev!

The patch corrects the selector used for indented comments in Bartik and follows the selector used by Classy.

Screenshots below:

LTR


 

RTL

catch’s picture

Issue tags: +rc target triage
xjm’s picture

Issue tags: -rc target triage +rc target

Discussed with the D8 committers and we agreed with making this an rc target.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

  • webchick committed a9247a5 on 8.0.x
    Issue #2512468 by bendev, munzirtaha, rudraram, chipway, emma.maria,...

Status: Fixed » Closed (fixed)

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