Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
These changes are coming from the feedback that we've received from the patch submission that we've made on 9/23/2020.
-
+++ b/core/themes/olivero/css/components/comments.pcss.css @@ -0,0 +1,245 @@ +.comments { + > .comment { ... + ~ .comment {
Could we provide a CSS class indicating whether the comment is on the main level or not instead of using CSS? Technically this is against BEM since block elements shouldn't depend on other elements on the page.
-
+++ b/core/themes/olivero/css/components/comments.pcss.css @@ -0,0 +1,245 @@ + .text-content { ... + .links {
Can we customize the markup so that either
.links
and.text-content
become their own blocks or they are rendered as elements inside.comment
?
Steps to reproduce
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#45 | interdiff-41-45.txt | 4.56 KB | mherchel |
#45 | 3173007-45.patch | 15.33 KB | mherchel |
#44 | interdiff-41-44.txt | 4.56 KB | mherchel |
#44 | 3173007-44.patch | 20.22 KB | mherchel |
#41 | interdiff-35-41.txt | 2.29 KB | mherchel |
Comments
Comment #2
proeungComment #3
kostyashupenkoComment #4
mherchelPatch applies and works great. I have a couple code questions:
Why are you breaking out the
.text-content
into its own code block here? Why not leave nested?Same question as above.
Comment #5
kostyashupenkoNo excuses :) but now i guess my patch is finished
Comment #6
mherchelThis looks great! I have a couple of pretty minor changes before it is RTBC. Thanks for working on this!
Should this be using the identical operator
===
?Let's change this class to
comment_text-content
, so it's obvious what it's doing within the CSS.Comment #7
kishor_kolekar CreditAttribution: kishor_kolekar as a volunteer and at QED42 for Drupal India Association commentedWorked on comment #6
Comment #9
mherchelIf we change the selector here, we also need to change the selector within the stylesheet (or it won't apply the styles).
Comment #10
kostyashupenkoAs i remember we already have
comment__text-content
selector somewhere in treeComment #11
komalk CreditAttribution: komalk at Srijan | A Material+ Company for Drupal India Association commentedComment #12
komalk CreditAttribution: komalk at Srijan | A Material+ Company for Drupal India Association commentedWorked on #9.
Review the patch.
Comment #13
komalk CreditAttribution: komalk at Srijan | A Material+ Company for Drupal India Association commentedforgot to add #6.1 in the previous patch.
Comment #14
komalk CreditAttribution: komalk at Srijan | A Material+ Company for Drupal India Association commentedComment #15
mherchelFrom @kostyashupenko in #10
Yep. You're completely correct. However, the selector isn't being used. It's on line 83 of core/themes/olivero/templates/content/comment.html.twig.
We should change this to
comment__content-wrapper
, since that seems to be this particular div's function. The rest of the patch in #14 is looking good.Comment #16
kishor_kolekar CreditAttribution: kishor_kolekar as a volunteer and at QED42 for Drupal India Association commentedComment #17
kishor_kolekar CreditAttribution: kishor_kolekar as a volunteer and at QED42 for Drupal India Association commentedWorked on Comment #15
Comment #18
kishor_kolekar CreditAttribution: kishor_kolekar as a volunteer and at QED42 for Drupal India Association commentedplease avoid the previous patch.
Comment #19
mherchel#18, we should change the instance of the current
comment__text-content
tocomment__text-wrapper
and then add the new class inComment #20
Santosh_Verma CreditAttribution: Santosh_Verma at Srijan | A Material+ Company for Drupal India Association commentedComment #21
Santosh_Verma CreditAttribution: Santosh_Verma at Srijan | A Material+ Company for Drupal India Association commented@mherchel I have checked patch #18, as per my understanding it's all done as you asked or maybe I am missing, can you please explain little bit more
Comment #22
mherchelThe patch in #18 adds the class `comment__text-wrapper`. Instead of doing this, we should be adding `comment__text-content`. But before we do that, we need to change the current name of `comment__text-content` (in the twig file) to `comment__text-wrapper`
Comment #23
paulocsComment #24
paulocsI created a new patch.
Comment #25
kostyashupenkoPrevious patch lgtm
Comment #26
raman.b CreditAttribution: raman.b at OpenSense Labs commentedRequired re-roll after #3117698: Allow PostCSS Plugin “Px to Rem” in core for Olivero theme
Comment #27
lauriiiWe could override
links--comment.html.twig
to add classes for these elements.Comment #28
raman.b CreditAttribution: raman.b at OpenSense Labs commentedOverriding twig templates and removing classes from preprocess
Comment #29
mherchelPatch in #28 looks perfect. Thanks!
Comment #30
mansoor20 CreditAttribution: mansoor20 as a volunteer and commentedLooks good. RTBC +1
Comment #31
mherchelPatch still applies and still looks good.
Comment #32
mherchelCredit looks good.
Comment #33
mherchelTalked to @lauriii within code sprint.
We need to add BEM classes onto the comment links (reply, etc)
Comment #34
proeungAs @mherchel noted, we need to apply classes to the
<li>
and<a>
tags to keep the consistency with BEM methodology for the unordered list. We might need to add a preprocess function to apply an attribute to the links array in order to expose these classes.Comment #35
mherchelUpdated the comment link selectors per the previous comments.
Note that while doing this, I discovered a number of the CSS styles were redundant, so I was able to remove them.
Comment #36
proeung@mherchel Thanks for submitting this revised patch and removing redundant styles. I just have minor feedback for the patch in #35.
A bit of nitpicking, but IMHO this class naming convention (
.comment__links-link
) doesn't fit the BEM tree. I would use.comment__links__link
use instead for link classname. Also, see below of an unordered list BEM tree.Comment #37
mherchelI'm pretty sure that we should not include each level in the class name. I was looking for Drupal documentation on that, but can't find it.
I did find this info from https://seesparkbox.com/foundry/bem_by_example
I'd love to get @lauriii's input on this.
Comment #38
kostyashupenkoMy 50 cents:
The classname
comment__links-link
looks kinda weird to me, so the element name islinks-link
, which is not obvious.The next classname, proposed in #36
comment__links__link
looks not obvious aswell, you can't have block of the block. The construction should be simple and common: block__element--modifierIf you want to handle another block, then it's better replace
comment__links__link
bycomment-links__link
, so blockName becomescomment-links
Also, about #35 - for templates
field--comment-body.html.twig
andlinks--comment.html.twig
not possible to avoid overriding templates and manage everything on hook?Comment #39
lauriiiI don't think
comment__links__link
is allowed. I'm fine with Icomment__links-link
or we could even simplify that tocomment__link
.Comment #40
bnjmnmcomment__link
as suggested in #39Since we're not dealing with levels beyond first, would is-parent be more accurate/useful?
If level is preferred, may be worth switching to "level-1" instead of "first-level" so it's consistent with nav class naming.
-C -C
when you generate it?git diff 9.2.x -C -C > patch.patch
? This may make it easier to distinguish the Olivero-specific changes to those templates vs the contents that were pasted in from a broader template.Re #38
The expectation for core is to do this in templates whenever possible. Details on the decision here: [#2325067]. The preprocess preference is an understandable one, though 🙂.
Comment #41
mherchelI ended up going with a
comment--level-1
class to keep consistency across the codebase (we do something similar for menus).Updated patch attached!
Comment #42
bnjmnmThe patch itself looks good so I had a look at what might need changing that isn't in the patch. Only spotted one thing:
Looks like there's a non-bem
.comment-picture
being added in a few templates and used for this styleIt seems like it should be possible to get rid of .comment-picture everywhere and not have to worry about BEM-ifying it. That would also address some of the classname confusion that is happening like in commment.html.twig, where there's a div with
.comment-picture
div with a child div that has.comment__picture
Comment #43
mherchelComment #44
mherchelUpdated patch that resolves #42 attached!
Comment #45
mherchelUgh. The last patch included the interdiff! Updated patch attached :D
Comment #46
proeungThe updated patch from #45 looks good.
RTBC +1
Comment #48
lauriiiCommitted bb59259 and pushed to 9.2.x. Thanks!