Refactor Claro's button stylesheet

I have addressed the issues from #11, #13.

1. Removed all RTL changes: Used margin-inline-start & margin-inline-end instead of margin-left, margin-right
2. Removed LTR comments.
3. Improved nesting
4. Removed importing variables.

Please review

single quote fix.

Reviewing #17

All lines appear to be nested correctly
Colors are replaced by variables

Looks good to me.

1. +++ b/core/themes/claro/css/components/button.pcss.css
   @@ -29,120 +29,116 @@
   +  margin-block-start: var(--space-m);
   +  margin-block-end: var(--space-m);
   

   margin-block: var(--space-m);

2. +++ b/core/themes/claro/css/components/button.pcss.css
   @@ -29,120 +29,116 @@
   +  margin-inline-start: 0;
   +  margin-inline-end: var(--space-s);
   

   margin-inline: 0 var(--space-s);

3. +++ b/core/themes/claro/css/components/button.pcss.css
   @@ -29,120 +29,116 @@
   +    margin: var(--space-s) var(--space-xs) var(--space-s) 0; /* LTR */
   

   There's no longer an RTL equivalent so this /* LTR */ should no be there

4. +++ b/core/themes/claro/css/components/button.pcss.css
   @@ -29,120 +29,116 @@
   +    margin-block-start: var(--space-xs);
   +    margin-block-end: var(--space-xs);
   

   use margin-block shorthand

5. +++ b/core/themes/claro/css/components/button.pcss.css
   @@ -29,120 +29,116 @@
   +    margin-inline-start: 0;
   +    margin-inline-end: var(--space-xs);
   

   use margin-inline shorthand

6. +++ b/core/themes/claro/css/components/button.pcss.css
   @@ -29,120 +29,116 @@
   +    margin-inline-start: -0.25em;
   +    margin-inline-end: 0;
   

   margin inline shorthand

7. +++ b/core/themes/claro/css/components/button.pcss.css
   @@ -29,120 +29,116 @@
   +    padding-inline-start: 0.25em;
   +    padding-inline-end: 0;
   

   use padding-inline shorthand

8. +++ b/core/themes/claro/css/components/button.css
   @@ -41,12 +41,15 @@
   +  margin-block-start: var(--space-m);
   +  margin-block-end: var(--space-m);
   

   these two lines can be

   margin-block: var(--space-m)
   </li>
   
   <li>
   <code>
   +++ b/core/themes/claro/css/components/button.css
   @@ -41,12 +41,15 @@
   +  margin-inline-start: 0;
   +  margin-inline-end: var(--space-s);
   

   These two lines can be margin-inline: 0 var(--space-s);

9. Also needs screenshots to confirm no regressions. If there's any way to demonstrate the new CSS is laoding in the screenshots, that's a bonus. It may not be possible due to the CSS compiling to the same thing, however.

Addressed all from #19. please review

The last submitted patch, 20: 3294002-20.patch, failed testing. View results →

Unrelated failure, restoring status.

Points appear to have been addressed

The patch looks pretty good. Someone will still need to test this thoroughly.

We have used https://github.com/zolhorvath/cd_tools in the past but I don't think it's Drupal 10 compatible 😐

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

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

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

Oh neat. Cloned the module and changed for D10 in core_version_requirements

1. +++ b/core/themes/claro/css/components/button.css
   @@ -81,7 +78,7 @@
   +  margin: var(--space-s) var(--space-xs) var(--space-s) 0;
   

   Need to use logical props

2. +++ b/core/themes/claro/css/components/button.css
   @@ -153,18 +140,15 @@ a.button--primary:active {
   -a.button--danger:hover,
   +.button--danger:active,
    a.button--danger:active {
   -  color: var(--button-fg-color--danger);
   +  background-color: var(--button--active-bg-color--danger);
   

   we need this rule

   a.button--danger:hover,
   a.button--danger:active {
     color: var(--button-fg-color--danger);
   }
   
   

   Otherwise danger links that are active have a black text color instead of white which makes them hard to read and not very pretty.

Addressed feedback from #28. please review

can we keep the rules as they were before? last patch changes the specificity of the background rule when we only want to override the color property.

I don't see where the last patch changes the specificity, but I do see some unneeded specificity in the patch. In addition,

• some of the nesting seems unnecessary
• There's an !important where the comments say that its for FF in high contrast mode. I can't see why. I'm going to test this out and remove if possible..
• CSS custom properties should be moved to this file and scoped to the element if possible.

I'm going to put a bit of work in now, and see how far I can get.

Adding testing HTML to IS

Updated patch attached.

A couple notes on the patch above:

• The special !important rule for Firefox in forced-colors mode is no longer necessary. Animated GIF is attached below.
• Some of the custom properties attached to :root are used in the Dropbutton component, so I couldn't move them into here without affecting that. We can do that at a later time.

Firefox in high contrast (also showing focus styles):

#34 looks good to me.

rerolling #34 to make the test pass.

Patch #34, does not apply anymore. I have attached the re-rolled patch.

#41 - Works for me
patch applied successfully
Thanks @Gauravvvv for the hard work!

Committed e5b998e and pushed to 11.x. Thanks!

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

This is a minor only change, removing tag for the followup.