User's image in comments overlaps with Layout Builder section

#2 Please review the patch

please review the patch.

Verified and tested patch#2. Patch applied successfully and looks good to me.

Testing Steps:

1. Install drupal 9.2.x-dev version
2. Go to Appearance -> Set Olivero theme as admin and default theme
3. Go to Content and create one article content type also post a comment.
4. Go to Home>Administration>Structure>Content types>Article,under layout options check layout builder.
5. Go to the article node, click on layout.
6. Scroll down to the place where comments are being posted.
7. You will see the user's image(circular one) overlapping with the layout builder section.
8. Now apply the patch and clear cache and again go to the article node, click on layout and verify now user's image(circular one) not overlapping with the layout builder section and displayed properly.

Testing Results:
After applying the patch user's image (circular one) not overlapping with the layout builder section and displayed properly.

#4 Recreated Patch

Applied patch #4 and it works fine.

Before patch;

After patch:

But there are some whitespace and CS issues in this patch.

3210703.4.patch:38: trailing whitespace.
[dir="ltr"] .layout-builder-block .comment .add-comment__picture,[dir="ltr"] 
3210703.4.patch:43: trailing whitespace.
[dir="rtl"] .layout-builder-block .comment .add-comment__picture,[dir="rtl"] 
Checking patch core/themes/olivero/css/components/comments.css...
Checking patch core/themes/olivero/css/components/comments.pcss.css...
Applied patch core/themes/olivero/css/components/comments.css cleanly.
Applied patch core/themes/olivero/css/components/comments.pcss.css cleanly.
warning: 2 lines add whitespace errors.

Fixed whitespace and CS issues in patch #4.

#12 Recreated patch

Kindly review the attached patch.

Thanks!

+++ b/core/themes/olivero/css/components/comments.pcss.css
@@ -235,3 +235,19 @@
+  & .comment {
...
+    & .add-comment__picture,

Isn't .comment unnecessary here? Also, I don't think this would work because .add-comment__picture isn't inside .comment.

Verified and tested patch#23. Patch applied successfully and looks good to me.

Moving to RTBC

#21 still needs to be addressed. I'm not sure why .add-comment__picture was added in the first place but removing it would be probably fine since it is never inside .comment, so the selector never matched anything.

@lauriii,
Can we keep it simple and remove few lines of code.
please review the patch.

We need to ensure that these fixes don't affect the output of the styling when the LB builder is not active.

@mherchel I think it won't affect since it's only written to fix issues inside LB which is working fine after patch. I checked on the node comment section it's working fine #23.

SS:

Thanks!

Normal priority bug under the normal issue priority definition:

Bugs for site visitors that do not interfere with site use, for example, visual layout issues.

Agree with @xjm.
And UI of the Image(circular one) is too close to the horizontal line. but it also does not interfere with site use.

Verified and tested patch #29.
Patch applied successfully and looks good to me.

Testing Steps:
# Go to Appearance:- Set Olivero theme
# Go to Content and create one article content type also post a comment.
# Go to Extend:- Install the layout builder option.
# Go to Home>Administration>Structure>Content types>Article,under layout options check layout builder.
# Go to the article node, click on layout.
# Scroll down to the place where comments are being posted.
# You will see the user's image(circular one) overlapping with the layout builder section

Expected Results:
# User's image(circular one) should not overlap with the layout builder section and displayed properly.

Actual Results:
# User's image(circular one) overlapping with the layout builder section.

Please refer attached screenshots.
Looks good to me.

I applied the latest patch from #29 and it breaks some stuff, so i decided to revert it and create new patch from scratch.

For local tests i have used Drupal 9.3.x + standard profile installation + default CT Article page with multiple comments.

I've found several bugs with alignment of comments on real pages between 700px - 1000px width of screen:

So you can see that comment picture is outside of viewport. To fix this little issue was not so obvious and easy thing, but now everything works fine on my side under all breakpoints + also LB comments were fixed (and looks ok under all breakpoints aswell).

There are two examples of comments on real page and LB page

Here is a patch btw

I have applied #35 patch and tested it with the hirechical index example.

Applied patch #16 working fine and giving expected result
Thanks for the patch
For ref sharing screenshots....

@kostyashupenko RTL the User's image in comments is overlapping with layout builder section.

Attached screenshot here.

Moved to Needs work

#40 Please review the patch

I have fixed the custom commands failed, Attached interdiff for same. Please review.

Thanks for the update. The indentation in the patch seems off to me, e.g.

 @media (min-width: 43.75rem) {
 
+[dir="ltr"] .comment {
+    padding-left: 5.625rem
+  }
+
+[dir="rtl"] .comment {
+    padding-right: 5.625rem
+  }
+  }

Also not sure that #28 was properly addressed:

#21 still needs to be addressed. I'm not sure why .add-comment__picture was added in the first place but removing it would be probably fine since it is never inside .comment, so the selector never matched anything.

Cleaned up title and the issue summary formatting and wording. Also added more information to the steps to reproduce. Still could use the full issue summary template.

I'm going to work on #42 and #28.

@kristen-pol Indeed the identantion is off on the comments.css file.

There are 10 places on the original file with a @media tag wrapping some css rules and their curly braces positions and identation is not consistent (differently from comments.pcss.css file which follows the coding standards).
I corrected the indentation just for the blocks added/edited by the patch to keep within this issue scope, maybe a follow up issue just for the CS after this is committed would be a good idea.

Also #28 was addressed in #23 (see the interdiff). The original problem (#21) was about the .add-comment__picture inside .comment which was correctly removed.

The patch bellow fixes the identations and CS on the introduced code.

Fixed CS errors.

Marking Needs Review again because test failures are unrelated.

1) Drupal\Tests\layout_builder\FunctionalJavascript\BlockFormMessagesTest::testValidationMessage
Failed asserting that a NULL is not empty.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/var/www/html/core/modules/layout_builder/tests/src/FunctionalJavascript/BlockFormMessagesTest.php:65
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

Getting above test failures in most of the issues.

Drupal 9.4.x-dev
Verified and tested patch #47.
Patch applied successfully and looks good for me.
Sharing screenshot for the reference.

Thanks for the updates and testing.

A frontend dev should take a look at the changes more carefully, but I reviewed interdiffs in #46 and #47.

The indentation problems appear to be addressed. One thing that was added in that IMO doesn't need to be added is the comment: /* Comment's padding-top */

+++ b/core/themes/olivero/css/components/comments.css
@@ -350,7 +350,7 @@
+    top: 2.25rem; /* Comment's padding-top */

@Kristen Pol Thank you for pointing that out. If it wasn't you I wouldn't notice the interdiff on #47 is actually wrong. If you download the patch #46 and the patch #47 and run interdiff 3210703-46.patch 3210703-47.patch > interdiff_3210703_46-47.txt you will get the interdiff attached here.

It means #47 corrected the 3 spaces on core/themes/olivero/css/components/comments.pcss.css but also reverted all other CS corrections I made on #46.

Here is a new patch with the spaces from core/themes/olivero/css/components/comments.pcss.css corrected, and all the corrections I did on #46 added back.

@Kristen Pol, also, regarding the addition of /* Comment's padding-top */ it just seems that I have added this comment because of the interdiff, but actually I reverted this change. This comment is already present on the original core code and the previous patch (#41) was removing it. As it seemed an unrelated change I just didn't delete the comment from the code when working on my patch.

Applied patch #52. Patch failed. No changes

Resolved CSpell errors.

Sorry, I'm feeling a bit embarrassed because at the time I worked on this issue I didn't know that we should not change the "file_name".css files, and inadvertently tried to "fix" comments.css causing my patch to fail and adding unnecessary noise to this issue's thread.

The correct approach is to change/update the .pcss.css version of whatever file we are working with, and then run yarn run build:css on the core folder, so yarn automatically updates the .css version of the file. (see documentation here).

Even if the .css file looks a bit off (in this issue's case, with inconsistent indentation, etc., etc.) we should keep it as is, and only update it through yarn when changing the .pcss.css equivalent.

@yogeshmpawar probably did this on his patch (thanks for that!), and now the patch not only fix the issue (as you can see on the pictures attached), but it's also free of errors.

Moving to RTBC.

Unrelated test fail, requeueing tests ans restoring status.

Another random failure.

Random failure again.

Before apply patch:

After apply patch:

@manojithape, what was the intent of the new patch? It's a good practice to always write a comment explaining what you did on your patch and to provide an interdiff between your patch and the last one. This helps other members on the community to know what you're doing.

Also, the patch #54 was working just fine. The "Needs work" tag was added automatically by a random fail. A further investigation on the test results (or reading the comments) could have helped to know this.

Sending back to RTBC as per patch #54.

CI is showing that the css is out of sync with the pcss, so this needs work

#68 Please review the patch

Before Patch

After Patch

Applied patch #68 applied successfully in drupal-9.5.x-dev.
For ref sharing screenshots...
We can move this ticket to RTBC.

Verified and tested #68 applied successfully and looks good to me
Testing steps-
1.Make Olivero the default theme in Appearance.
2.Enable Layout Builder module in Extend.
3.Create an Article page with comments.
4.Go to Home > Administration > Structure > Content types > Article
5.Under Manage Display tab go to Layout options
6.Check Use Layout Builder and Allow each content item to have its layout customized. then save.
7.Go to the Article page and click on the Layout tab.
8.Scroll down to the comments section.

Actual Result: You will see the user's circular image overlapping with the Layout Builder section.
Expected Result: User's image should be aligned correctly within the comment.
Can be move to RTBC

I'm really indifferent if this even needs to be fixed. I'm recategorizing as minor.

If we do fix this, we need to ensure the non-layout builder UI is not affected. The current patch affect the comments when the LB UI is not in use.

I have created this patch for 10.1.x-dev and its working fine. Please verify.

User's profile image in comments section overlaps with in Article has been fixed. Please review. Thanks !!!

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

comments.pcss.css has been updated and User's profile image in comments overlaps with Layout Builder section issue has been resolved

I've tested MR 8645 on Drupal 11.x
The MR is applied cleanly...

Test Result:
The User's image in the comments overlaps with the Layout Builder section issue is resolved and visually looks good.
Also, the Non-Layout UI looks good.

Attaching Scrrenshots

RTBC+1

I read the issue summary, which is incomplete, there is no proposed resolution. And as, an issue that affects the UI there should be screenshots available from the issue summary. I am restoring the template and will update as I continue. Thanks you to andregp for summarizing the relevant changes and what comments have been addressed.

I think everything has been addressed here, but worth checking that #72 is complete. Also an Olivero maintainer commented in #72 that they are "indifferent" about this being changed.

Leaving at RTBC

Updated attributions

I reviewed the issue and all the different solutions change the design of the comment section one way or another. Changing the design of the comment section is not in the scope of this issue.

The issue is that inside the LB UI the icon is outside the LB section. This is by design that the picture is out of the normal flow of the page, it makes sense that it is also out of the flow of the LB interface. If we make it fit inside the LB the preview would not be accurate anymore.

Moving the comment element to the right to avoid this, but it breaks the intended design of the page. If the overlap is problematic it's always possible to disable the content preview with the checkbox on top of LB UI. Because we do not want to change the design of the comment and there is a solution to avoid this I'm closing this as works as designed.

Thanks to everyone who worked on this over the years. I wish we caught this one earlier to avoid spending so much time on this.