Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
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. |
Comment | File | Size | Author |
---|---|---|---|
#61 | rtl.png | 70.05 KB | emma.maria |
#61 | ltr.png | 73.75 KB | emma.maria |
#59 | indented_styles_for-2512468-59.patch | 490 bytes | bendev |
#57 | indented_styles_for-2512468-57.patch | 614 bytes | bendev |
#52 | Screen-Shot-2015-10-06-at-21.38.55.png | 182.41 KB | emma.maria |
Comments
Comment #1
munzirtaha CreditAttribution: munzirtaha as a volunteer commentedAttached is a patch to clean it up.
Comment #2
cchanana CreditAttribution: cchanana commentedComment #3
cchanana CreditAttribution: cchanana commented@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.
Comment #4
munzirtaha CreditAttribution: munzirtaha as a volunteer commented@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.
Comment #5
trevorkjorlien CreditAttribution: trevorkjorlien as a volunteer and at Floe design + technologies commentedApplied the patch in #4 and comments appear indented correctly in both LTR and RTL.
Comment #6
trevorkjorlien CreditAttribution: trevorkjorlien as a volunteer and at Floe design + technologies commentedComment #7
trevorkjorlien CreditAttribution: trevorkjorlien as a volunteer and at Floe design + technologies commentedComment #8
Anonymous (not verified) CreditAttribution: Anonymous as a volunteer commentedUpdating the issue tag to include a hashless DrupalNorth2015 on behalf of the Drupal North sprinting group.
Comment #9
Manjit.Singh@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.
Comment #10
Manjit.SinghComment #11
mparker17Removing old hashed tag.
Comment #12
munzirtaha CreditAttribution: munzirtaha as a volunteer commented@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.
Comment #13
LewisNyman CreditAttribution: LewisNyman at Wunder commented@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.
Comment #14
munzirtaha CreditAttribution: munzirtaha as a volunteer commented@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.
Comment #15
Manjit.Singh@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 ?
Comment #16
Manjit.Singhsetting back to needs review as we need a screenshots for intended comments.
Comment #17
emma.mariaI cannot see why we are removing Bartik styles which are different to Classy.
Bartik sets 40px on the left in it's styles.
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.
Comment #18
rudraram CreditAttribution: rudraram at Axelerant commentedPatch in #4 is getting rejected.
Comment #19
emma.mariaThe patch in #4 needs a reroll plus the work added to it mentioned in #17.
Comment #20
rudraram CreditAttribution: rudraram at Axelerant commented@emma.maria Updated the patch as per your suggestion in #17 and styles are getting applied from the Bartik's comments.css file.
Comment #21
rudraram CreditAttribution: rudraram at Axelerant commentedComment #22
Manjit.SinghNeed some screenshots after #20.
Comment #23
fil00dl CreditAttribution: fil00dl at Skilld commentedthis issue seems fixed (please find screenshots in attachment)
Comment #24
nicolas@webstanz.be CreditAttribution: nicolas@webstanz.be commentedWe are working on in Barcelona with joelpitter & chipway
Comment #25
jim005 CreditAttribution: jim005 commentedComment #26
jim005 CreditAttribution: jim005 commentedComment #27
chipway CreditAttribution: chipway at Chipway commentedWorked on the issue at Barcelona 2015 with jim005, JBO, rbrissaud,. It looks RTBC.
Comment #28
jim005 CreditAttribution: jim005 at WebSenso commentedTested in Barcelona DrupalCon 2015, added screenshots (before / after) patch in summary section and in comment.
Comment #29
rbrissaud CreditAttribution: rbrissaud commentedWorked on the issue at Barcelona 2015 with jim005, JBO, chipway. It looks RTBC.
Comment #30
elchiconube CreditAttribution: elchiconube as a volunteer and commentedIt's woking well after patch here in DrupalCon Barcelona
Comment #31
davidhernandezConsidering this also changes RTL styles can we get right to left screenshots to verify it is ok?
Comment #32
davidhernandezComment #35
chipway CreditAttribution: chipway at Chipway commentedWorking 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.
Comment #36
chipway CreditAttribution: chipway at Chipway commentedComment #37
chipway CreditAttribution: chipway at Chipway commentedComment #38
chipway CreditAttribution: chipway at Chipway commentedComment #39
chipway CreditAttribution: chipway at Chipway commentedOups. images too big.
Comment #40
chipway CreditAttribution: chipway at Chipway commentedComment #41
chipway CreditAttribution: chipway at Chipway commentedAs last test passed (https://qa.drupal.org/pifr/test/1139948) the patch looks ready to go RTBC.
Comment #42
chipway CreditAttribution: chipway at Chipway commentedWho could RTBC https://www.drupal.org/node/2512468#comment-10384197 @ced_cht @eboux @nichautenauve @gaelgosset @Gromit06 @WebSenso @benoitdevos @Creatile #drupalconeur
Comment #43
bendev CreditAttribution: bendev at WebstanZ commentedpatch tested
indentation ok
Comment #46
chipway CreditAttribution: chipway at Chipway commentedLast test passed with 0 failure.
(Previous fails seemed to be because of Amazon services being unavailable)
Comment #47
emma.mariaThanks 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.
There should be no visual changes from applying this change.
Comment #48
bendev CreditAttribution: bendev at WebstanZ commentedCSS selector updated according to #47 request
Comment #49
emma.mariaComment #50
Manjit.SinghWe need a manual testing after applying the changes of #47.
Comment #51
meenakshi.r CreditAttribution: meenakshi.r as a volunteer and at Srijan | A Material+ Company commentedPatch in #48 looks good.
Before applying patch.
After applying patch.
Comment #52
emma.mariaI 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.
Comment #53
emma.mariaRelated bug issue that postpones this one... #2581407: Remove the obsoletely named Classy library attached to Bartik's comment.html.twig as Bartik uses its own styles. .
Comment #54
bendev CreditAttribution: bendev at WebstanZ commented@emma.maria are you sure ? I tested it again tonight and it still worked with the patch...
Comment #55
emma.mariaOK 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...
Please put back the margin-left: 0 on the RTL styles to cancel out the LTR styles.
Comment #56
emma.mariaComment #57
bendev CreditAttribution: bendev at WebstanZ commentedhere it is
Comment #58
emma.mariaOops there is a typo! Also can we put back the
.comment .links {
code back in also. That has nothing to do with this issue.Comment #59
bendev CreditAttribution: bendev at WebstanZ commentedsorry for the typo
Comment #60
bendev CreditAttribution: bendev at WebstanZ commentedComment #61
emma.mariaPerfect! 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
Comment #62
catchComment #63
xjmDiscussed with the D8 committers and we agreed with making this an rc target.
Comment #64
webchickCommitted and pushed to 8.0.x. Thanks!