Problem/Motivation

This is part of the CSS modernization initiative. This is intended to be a straightforward second issue.

The first issue was regarding the button stylesheet.

Steps to reproduce

The stylesheet at https://git.drupalcode.org/project/drupal/-/blob/10.0.x/core/themes/claro/css/components/button.pcss.css needs to be refactored to make use of modern CSS and Drupal core's PostCSS tooling.

Proposed resolution

Use CSS Logical Properties where appropriate
Use CSS nesting where appropriate

Remaining tasks

User interface changes

None. There should be no visual differences.

Testing HTML

<h2>Regular</h2>
<button class="button">Lorem, ipsum dolor.</button><br>
<a href="#" class="button">Lorem, ipsum dolor.</a><br>
<input type="submit" class="button" value="Lorem ipsum dolor"><br>

<h2>Small</h2>
<button class="button button--small">Lorem, ipsum dolor.</button><br>
<a href="#" class="button button--small">Lorem, ipsum dolor.</a><br>
<input type="submit" class="button button--small" value="Lorem ipsum dolor"><br>

<h2>Extra Small</h2>
<button class="button button--extrasmall">Lorem, ipsum dolor.</button><br>
<a href="#" class="button button--extrasmall">Lorem, ipsum dolor.</a><br>
<input type="submit" class="button button--extrasmall" value="Lorem ipsum dolor"><br>

<h2>Action</h2>
<button class="button button--action">Lorem, ipsum dolor.</button><br>
<a href="#" class="button button--action">Lorem, ipsum dolor.</a><br>
<input type="submit" class="button button--action" value="Lorem ipsum dolor"><br>

<h2>primary</h2>
<button class="button button--primary">Lorem, ipsum dolor.</button><br>
<a href="#" class="button button--primary">Lorem, ipsum dolor.</a><br>
<input type="submit" class="button button--primary" value="Lorem, ipsum dolor."><br>

<h2>danger</h2>
<button class="button button--danger">Lorem, ipsum dolor.</button><br>
<a href="#" class="button button--danger">Lorem, ipsum dolor.</a><br>
<input type="submit" class="button button--danger" value="Lorem, ipsum dolor."><br>

<h2>Disabled</h2>
<button disabled class="button">Lorem, ipsum dolor.</button><br>
<button class="button is-disabled">Lorem, ipsum dolor.</button><br>
<a href="#" class="button is-disabled">Lorem, ipsum dolor.</a><br>
<input type="submit" disabled class="button" value="Lorem ipsum dolor"><br>
<input type="submit" class="button is-disabled" value="Lorem ipsum dolor"><br>

<h2>Link</h2>
<button class="link">Lorem, ipsum dolor.</button><br>
<input type="submit" class="link" value="Lorem ipsum dolor"><br>
CommentFileSizeAuthor
#41 3294002-41.patch11.03 KBgauravvvv
#39 3294002-39.patch588.51 KBstanzin
#35 ff-buttons.gif105.35 KBmherchel
#34 interdiff-29-34.txt8.75 KBmherchel
#34 3294002-34.patch11 KBmherchel
#29 interdiff-26_29.txt1.55 KBgauravvvv
#29 3294002-29.patch10.41 KBgauravvvv
#27 Screen Shot 2023-03-03 at 10.01.25 AM.png87.17 KBsmustgrave
#27 Screen Shot 2023-03-03 at 10.01.19 AM.png198.12 KBsmustgrave
#26 interdiff.txt503 byteslauriii
#26 3294002-26.patch10.35 KBlauriii
#25 3294002-nr-bot.txt1.57 KBneeds-review-queue-bot
#24 interdiff.txt2.33 KBlauriii
#24 3294002-25.patch10.35 KBlauriii
#20 interdiff-17_20.txt3.33 KBgauravvvv
#20 3294002-20.patch9.44 KBgauravvvv
#17 interdiff-16_17.txt436 bytesgauravvvv
#17 3294002-17.patch9.53 KBgauravvvv
#16 interdiff-12_16.txt9.15 KBgauravvvv
#16 3294002-16.patch9.53 KBgauravvvv
#14 3294002-14.patch12.01 KBRatan Priya
#14 interdiff_12-14.txt912 bytesRatan Priya
#12 interdiff_10-12.txt19.21 KBshivam-kumar
#12 3294002-12.patch12.02 KBshivam-kumar
#10 3294002-10.patch22.89 KBsandeepsingh199
#7 3294002-version10.0.x.patch23.59 KBaditya4478
#5 3294002-10.0.x.patch9.18 KBaditya4478
#4 3294002-10.0.x.patch11.77 KBaditya4478
#3 3294002.9.5.x.patch8.21 KBaditya4478
#2 3294002-9.5.x-version.patch10.84 KBaditya4478

Issue fork drupal-3294002

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

sasanikolic created an issue. See original summary.

aditya4478’s picture

aditya4478 commented
Status: Active » Needs review
StatusFileSize
new 3294002-9.5.x-version.patch10.84 KB

Igore this patch

aditya4478’s picture

aditya4478 commented
StatusFileSize
new 3294002.9.5.x.patch8.21 KB

No block for IE in this file. It's good to go for review.

aditya4478’s picture

aditya4478 commented
Version: 9.5.x-dev » 10.0.x-dev
StatusFileSize
new 3294002-10.0.x.patch11.77 KB

Test Fails :(
I will attach new patch

aditya4478’s picture

aditya4478 commented
StatusFileSize
new 3294002-10.0.x.patch9.18 KB
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs review » Needs work

Set to Needs Work because patch doesn't apply.

aditya4478’s picture

aditya4478 commented
StatusFileSize
new 3294002-version10.0.x.patch23.59 KB
ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Parent issue: » #3324398: [META] Update Claro CSS with new coding standards
ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Issue summary: View changes
sandeepsingh199’s picture

sandeepsingh199 commented
StatusFileSize
new 3294002-10.patch22.89 KB

rerolling patch #7 for D10.1.x

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

This patch needs a lot of work. We don't need all of those CSS variables re-rolled, in addition the formatting is way off.

Lets start from a blank slate instead of working off the current patch. We

shivam-kumar’s picture

shivam-kumar commented
StatusFileSize
new 3294002-12.patch12.02 KB
new interdiff_10-12.txt19.21 KB

Worked on patch #10 Errors, removed unwanted all CSS variables,(as mentioned in comment #11) fixed as per coding standard.
Attached the interdiff file.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

Thanks for the work, but this still needs work. We don't need to import the variables as they're natively supported by the browser. In addition, we need to use CSS logical properties.

Ratan Priya’s picture

Ratan Priya commented
Status: Needs work » Needs review
StatusFileSize
new interdiff_12-14.txt912 bytes
new 3294002-14.patch12.01 KB

Fixed CCF for #12.
Please review.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs review » Needs work

Fixed CCF for #12.

Please see my previous comment. This needs work (not a reroll)

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
StatusFileSize
new 3294002-16.patch9.53 KB
new interdiff-12_16.txt9.15 KB

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

gauravvvv’s picture

gauravvvv commented
StatusFileSize
new 3294002-17.patch9.53 KB
new interdiff-16_17.txt436 bytes

single quote fix.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Reviewing #17

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

Looks good to me.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs screenshots 
  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.
gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
StatusFileSize
new 3294002-20.patch9.44 KB
new interdiff-17_20.txt3.33 KB

Addressed all from #19. please review

Status: Needs review » Needs work

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

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review

Unrelated failure, restoring status.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Points appear to have been addressed

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 3294002-25.patch10.35 KB
new interdiff.txt2.33 KB

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 😐

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3294002-nr-bot.txt1.57 KB

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.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs work » Needs review
StatusFileSize
new 3294002-26.patch10.35 KB
new interdiff.txt503 bytes
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs screenshots
StatusFileSize
new Screen Shot 2023-03-03 at 10.01.19 AM.png198.12 KB
new Screen Shot 2023-03-03 at 10.01.25 AM.png87.17 KB

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

1
2

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Needs work 
  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.

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
StatusFileSize
new 3294002-29.patch10.41 KB
new interdiff-26_29.txt1.55 KB

Addressed feedback from #28. please review

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Needs work

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.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

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.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Issue summary: View changes

Adding testing HTML to IS

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Issue summary: View changes
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Version: 10.0.x-dev » 10.1.x-dev
Status: Needs work » Needs review
StatusFileSize
new 3294002-34.patch11 KB
new interdiff-29-34.txt8.75 KB

Updated patch attached.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
StatusFileSize
new ff-buttons.gif105.35 KB

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):

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

#34 looks good to me.

shivam2812 made their first commit to this issue’s fork.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

stanzin’s picture

stanzin commented
StatusFileSize
new 3294002-39.patch588.51 KB

rerolling #34 to make the test pass.

aditya4478’s picture

aditya4478 commented
Status: Reviewed & tested by the community » Needs work
gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
StatusFileSize
new 3294002-41.patch11.03 KB

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

zeeshan_khan’s picture

zeeshan_khan commented
Status: Needs review » Reviewed & tested by the community

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

  • lauriii committed e5b998eb on 11.x 
    Issue #3294002 by Gauravvvv, Aditya4478, lauriii, mherchel, smustgrave,...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Fixed

Committed e5b998e and pushed to 11.x. Thanks!

Status: Fixed » Closed (fixed)

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

quietone’s picture

quietone commented
Issue tags: -Needs followup

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