[Code Review] Provide a BEM methodology to comments.pcss.css partial

Patch applies and works great. I have a couple code questions:

+++ b/core/themes/olivero/css/components/comments.pcss.css
@@ -73,45 +71,45 @@
+.comment .text-content {

Why are you breaking out the .text-content into its own code block here? Why not leave nested?

+++ b/core/themes/olivero/css/components/comments.pcss.css
@@ -73,45 +71,45 @@
+.comment .links {

Same question as above.

No excuses :) but now i guess my patch is finished

This looks great! I have a couple of pretty minor changes before it is RTBC. Thanks for working on this!

1. +++ b/core/themes/olivero/olivero.theme
   @@ -337,6 +337,10 @@ function olivero_preprocess_field(&$variables) {
   +    if ($variables['entity_type'] == 'comment') {
   

   Should this be using the identical operator ===?

2. +++ b/core/themes/olivero/olivero.theme
   @@ -337,6 +337,10 @@ function olivero_preprocess_field(&$variables) {
   +      $variables['attributes']['class'][] = 'comment__text';
   

   Let's change this class to comment_text-content, so it's obvious what it's doing within the CSS.

Worked on comment #6

+++ b/core/themes/olivero/olivero.theme
@@ -338,8 +339,8 @@
+      $variables['attributes']['class'][] = 'comment__text-content';

If we change the selector here, we also need to change the selector within the stylesheet (or it won't apply the styles).

As i remember we already have comment__text-content selector somewhere in tree

Worked on #9.
Review the patch.

forgot to add #6.1 in the previous patch.

From @kostyashupenko in #10

As i remember we already have comment__text-content selector somewhere in tree

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.

Worked on Comment #15

please avoid the previous patch.

#18, we should change the instance of the current comment__text-content to comment__text-wrapper and then add the new class in

@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

The 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`

I created a new patch.

Previous patch lgtm

Required re-roll after #3117698: Allow PostCSS Plugin “Px to Rem” in core for Olivero theme

+++ b/core/themes/olivero/css/components/comments.pcss.css
@@ -73,45 +71,49 @@
+  & a {
...
+  & li {

+++ b/core/themes/olivero/olivero.theme
@@ -584,3 +588,10 @@ function olivero_preprocess_search_result(&$variables) {
+function olivero_preprocess_links__comment(&$variables) {
+  $variables['attributes']['class'][] = 'comment__links';
+}

We could override links--comment.html.twig to add classes for these elements.

Overriding twig templates and removing classes from preprocess

Patch in #28 looks perfect. Thanks!

Looks good. RTBC +1

Patch still applies and still looks good.

Credit looks good.

Talked to @lauriii within code sprint.

We need to add BEM classes onto the comment links (reply, etc)

Updated 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.

I'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

If your component has child elements several levels deep, don’t try to represent each level in the class name. BEM is not intended to communicate structural depth. A BEM class name representing a child element in the component should only include the base/block name and the one element name.

I'd love to get @lauriii's input on this.

My 50 cents:
The classname comment__links-link looks kinda weird to me, so the element name is links-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--modifier

If you want to handle another block, then it's better replace comment__links__link by comment-links__link, so blockName becomes comment-links

Also, about #35 - for templates field--comment-body.html.twig and links--comment.html.twig not possible to avoid overriding templates and manage everything on hook?

I don't think comment__links__link is allowed. I'm fine with I comment__links-link or we could even simplify that to comment__link.

1. I prefer simplifying to comment__link as suggested in #39

2. +++ b/core/themes/olivero/templates/content/comment.html.twig
   @@ -68,6 +68,7 @@
        'js-comment',
   +    not parent_comment ? 'comment--first-level',
        status != 'published' ? 'comment--' ~ status,
   

   Since 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.

3. For the next patch, could you add -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.
4. I assume you've already noticed the CS issue in olivero.theme that custom commands caught.

Re #38

field--comment-body.html.twig and links--comment.html.twig not possible to avoid overriding templates and manage everything on hook?

The expectation for core is to do this in templates whenever possible. Details on the decision here: #2325067: CSS classes being moved from preprocess to Twig templates. The preprocess preference is an understandable one, though 🙂.

I ended up going with a comment--level-1 class to keep consistency across the codebase (we do something similar for menus).

Updated patch attached!

The 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 style

.add-comment__picture-wrapper.comment-picture {
  inset-block-start: calc(var(--line-height-base) + var(--sp0-5));
}

It 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

<div class="comment__picture-wrapper comment-picture">
    <div class="comment__picture comment-picture__image">
      {{ user_picture }}
    </div>
  </div>

Updated patch that resolves #42 attached!

Ugh. The last patch included the interdiff! Updated patch attached :D

Committed bb59259 and pushed to 9.2.x. Thanks!