Refactor Claro's vertical-tabs stylesheet

Refactored vertical-tabs.pcss.css I have improved nesting and optimised the code. Please review

Removed patch1, from comment #2.

Think more nesting could be done here. Too many spots to write out.

Also still see reference to [dir="rtl"] can any of those be replaced?

The .css file changed so screenshots in the issue summary will be needed.

Fixed coding standard issues. Attached patch with interdiff. please review

here are the before/after patch screenshots.

Before

After

The last submitted patch, 6: 3332729-6.patch, failed testing. View results →

Unrelated failure. Restoring status.

Thanks

The last submitted patch, 6: 3332729-6.patch, failed testing. View results →

Random failure

The last submitted patch, 6: 3332729-6.patch, failed testing. View results →

Random failure.

The last submitted patch, 6: 3332729-6.patch, failed testing. View results →

+++ b/core/themes/claro/css/components/vertical-tabs.pcss.css
@@ -27,10 +26,10 @@
+  @nest [dir="rtl"] & {

@@ -114,58 +109,45 @@
+  @nest [dir="rtl"] & {
...
+  @nest &:focus {
...
+  @nest &:hover {
...
+  @nest &::before {

@@ -183,13 +165,12 @@
+  @nest [dir=rtl] & {

@@ -229,27 +209,23 @@
+  @nest [dir="rtl"] & {

Need to remove all the @nest

Working on it

Note that the @nest is needed here because the ampersand & comes after the rule. If it becomes before the rule, then it's not needed

Yes mherchel explained it after I posted here. Ok with keeping them. Will re review later today.

Putting back to RTBC per #18.

+++ b/core/themes/claro/css/components/vertical-tabs.css
@@ -136,59 +129,43 @@
 [dir="rtl"] .vertical-tabs__menu-link {
-  padding-right: calc(var(--space-l) - var(--vertical-tabs-menu-link--active-border-size));
-  padding-left: var(--space-s);
   border-width: var(--vertical-tabs-border-size) var(--vertical-tabs-menu-link--active-border-size) var(--vertical-tabs-border-size) 0;
   border-radius: 0 var(--vertical-tabs-border-radius) var(--vertical-tabs-border-radius) 0;
 }

@@ -206,13 +183,11 @@
+[dir=rtl] :is(.vertical-tabs__menu-item.is-selected .vertical-tabs__menu-link::before) {
   border-radius: 0 var(--base-border-radius) var(--base-border-radius) 0;
 }

We could use the logical props for border width/radius and remove a couple of rtl specific styles, border radius require to set all 4 sides explicitly but it's possible

Working on it to replace normal css to logical properties

Add the logical properties for
1. Margin,
2. Padding,
3. Border,
4. Border-radius

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

Rerolling the patch #23 failed due to linting issue.

Please provide an interdiff. The size of the patch went up noticeably so would be curious for that.

@smustgrave Interdiff added

Patch has build errors.

I have updated the logical properties. Attached interdiff for same. please review

Thanks @Gauravvvv

Patch not applying atm 😎

Please review.

@bnjmnm i have tested the patch # 32, and it's applying clearly

Drupal : 10.1.x

santosh.verma@FVFYJ11AHV2D drupal % git apply -v 3332729-32.patch 
Checking patch core/themes/claro/css/components/vertical-tabs.css...
Checking patch core/themes/claro/css/components/vertical-tabs.pcss.css...
Applied patch core/themes/claro/css/components/vertical-tabs.css cleanly.
Applied patch core/themes/claro/css/components/vertical-tabs.pcss.css cleanly.

Re @Santosh_Verma #38 I reached my conclusion of the patch not applying because the Drupal.org testbot literally says "Patch Failed to Apply" in comment #32.

This is a really helpful way to determine if a patch applies without having to do anything locally, and rules out false positives that can happen from situation such as not pulling the most recent changes before attempting to apply the patch.

Either way, looks like @rpayanm has created an MR that addresses the merge conflict.

Back to RTBC.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch 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.

Please review.

Reroll seems good!

Added some comments to the MR

Addressed feedbacks on MR. please review

reviewing it

I have verified the MR changes in #46 asked in #45 all are points are addressed
2. I think margin: 0 is simpler and acceptable- addressed
3. Do we need @nest here? (@nest &:focus )- @nest removed
4. Do we need @nest here? (@nest &:hover )- @nest removed
5. Do we need @nest here? (@nest &::before {) - @nest removed

Tested the impact on the site nothing is breaking looks similar as screenshot attached in comment #6 that's why not adding before after screenshot :)

Fixed, CI issue.

Threads appear to be addressed.

Committed 1025718 and pushed to 11.x. Thanks!

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