Remove hardcoded CSS values, use css variables instead

I've converted as much as I can to using variables. I've left some hard coded values as they are "special numbers" and only available in one or two places in the CSS.

I'd like to get this much merged if we could as it's a huge improvement and we can see if we can amend the designs a little bit in a follow up to rationalise some of the magic numbers values.

Build error

But will keep an eye out as this looks good to me.

It's probably a fair failure. My copy/replace changed values in the /css/class directory. Maybe we should leave that directory alone.

Build error - command failed

Was just passing by and was amazed why our spellchecker cspell not only didn't know the word "grey", but also outright forbids it's usage.
Then I found #3155258: Use American English spelling of "gray" → .

@Spokje good find, but crazy we can't actually use it even in a variable name.

crazy we can't actually use it even in a variable name.

Mumbles something about crazy Yankees

Fully agreed, but that's the way it is :)

Close!

profiles/demo_umami/themes/umami/css/components/forms/buttons.css
45:16 ✖ Unexpected unknown function "-var" function-no-unknown
51:33 ✖ Unexpected unknown function "-var" function-no-unknown

Looks good!

Everything seems good only minor miss. In the core/profiles/demo_umami/themes/umami/css/components/blocks/disclaimer/disclaimer.css file, margin-left variable should be --spacing-smaller

- margin-left: 0.5rem; /* LTR */
+ margin-left: var(--spacing-small); /

Thanks.

Seems like the svg changes are not working as intended. Some of those files are loaded as background images and (at least on FF) the color is not picked up properly.

Maybe using a fallback in the svg files would do the trick.

Hi nod → ,
as per comment #17, if used the SVG images as background images and use the variables to fill the icon color it's not working (it's because they not get the variable source).

solution working fine
we can redeclare the variable in the SVG file, this will be working fine.

<svg id="Layer_1" data-name="Layer 1" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 600 600"><defs><style>
:root {
  --color-primary: #008068;
}
.cls-1{fill:var(--color-primary);}</style></defs><path class="cls-1" d="M299.29 118.57c-120.29 0-217.86 97.61-217.86 217.86a215.64 215.64 0 0 0 48.81 137.22 13.8 13.8 0 0 0 19.76 2.12c5.66-5 7.07-14.15 2.12-19.81a187.92 187.92 0 0 1-20.51-31.83l45.27-18.39a14.12 14.12 0 1 0-10.61-26.17l-46 18.39c-5-14.15-8.49-29-9.2-44.56h48.1a14.15 14.15 0 0 0 0-28.29h-48.74a165.28 165.28 0 0 1 8.49-46l43.85 18.39c2.12.71 3.54 1.41 5.66 1.41a14 14 0 0 0 12.73-8.49 14.36 14.36 0 0 0-7.78-18.39l-43.85-18.39A204.84 204.84 0 0 1 155 214.77l32.54 32.54a13.68 13.68 0 0 0 19.81 0 13.68 13.68 0 0 0 0-19.81l-33.24-33.24A211.56 211.56 0 0 1 213 167.38l17.68 42.44c2.12 5.66 7.78 8.49 13.44 8.49 2.12 0 3.54 0 5.66-1.41a15.46 15.46 0 0 0 3.54-2.12c-1.41 12-2.83 30.42-2.83 58 0 33.24.71 74.27 3.54 86.29a49.32 49.32 0 0 0 48.1 38.2 54.6 54.6 0 0 0 10.61-1.41c26.88-5.66 43.85-31.83 38.2-58-2.12-12-18.39-49.51-31.83-79.22-12.73-28.29-21.22-45.27-27.59-55.17 2.12 2.12 5.66 2.83 8.49 2.83a14.19 14.19 0 0 0 14.15-14.15v-44.58a202.27 202.27 0 0 1 46.68 9.2l-17.68 42.44a14.36 14.36 0 0 0 7.78 18.39c2.12.71 3.54 1.41 5.66 1.41a14 14 0 0 0 12.73-8.49L387 168.08a182.4 182.4 0 0 1 38.2 26.17L392 227.5a13.68 13.68 0 0 0 0 19.81 13.68 13.68 0 0 0 19.81 0l32.54-32.54c9.9 12 19.1 25.46 25.46 39.61l-43.85 18.39a14.36 14.36 0 0 0-7.78 18.39c2.12 5.66 7.78 8.49 13.44 8.49 2.12 0 3.54 0 5.66-1.41l43.85-17.68a233 233 0 0 1 8.49 45.27h-48.1a14.15 14.15 0 0 0 0 28.29h48.1a199.17 199.17 0 0 1-9.9 45.27l-45.27-19.1a14.12 14.12 0 0 0-10.61 26.17l44.56 18.39a207.78 207.78 0 0 1-21.93 33.24 13.8 13.8 0 0 0 2.12 19.81c2.83 2.12 5.66 3.54 9.2 3.54a12.73 12.73 0 0 0 10.61-5 217.33 217.33 0 0 0 50.22-139.34c-.76-120.92-99.08-218.53-219.33-218.53zm24 226.35a20.66 20.66 0 0 1-16.27 24c-11.32 2.12-22.63-5-24.76-15.56-2.12-12-3.54-71.44-2.12-110.34 16.32 35.41 40.37 90.58 43.2 101.9zM271.71 188.6h-2.83c-5 .71-8.49 2.83-11.32 9.9l-17.68-42.44a196.33 196.33 0 0 1 46-9.2v46c0 1.41 0 2.83.71 3.54-7.1-7.8-10.59-7.8-14.88-7.8z"/>  <circle class="cls-1" cx="300.24" cy="339.59" r="9.2" transform="rotate(-11.9 300.247 339.58)"/></svg>

2. solution - working (looks more better, it's variables control generic way)

Thanks @Harish1688 for working on this.

I've added a new commit to remove all the Adobe Illustrator classes from our SVGs. I hadn't realised they were still there to be honest, and should not be. They are too generic and if used inline .cls-1 in one SVG will override .cls-1 in another SVG if they are on the same page.

All the SVGs are now using the same approach of having their fill colour set via a CSS hex value.

Hi markconroy → ,
Thanks for this point, but instead of fill the hard value in the fill=" " attribute, we may declare the variables here to like .

<svg id="Layer_1" data-name="Layer 1" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 600 600"><defs>
<link xmlns="http://www.w3.org/1999/xhtml" rel="stylesheet" href="../../css/variables.css" type="text/css"/>
</defs><path fill="var(--color-primary)" d=" "/><path fill="var(--color-primary)" d=""/></svg>

In that case we will not face any css .cls-1 classes conflict issues on generic approach. and variables also can be used.

There's 2 issues with that:

1. We need to go through every CSS value in each SVG, which could be tricky (only the SVGs exported via Illustrator have this .cls-1 issue, which is why this only showed up in 2 SVGs)
2. When I tried to add fill="var(--color-primary) to SVGs last night, the variable wasn't picked up (perhaps because these SVGs are used as background images rather than inline.

Let's get this MR merged with the current approach and we can file a follow-up issue to look at the SVGs more closely later.

Hi markconroy,

1. We need to go through every CSS value in each SVG, which could be tricky -- we could this way only, because SVG called as background image so css will not here. (masking css property not a proper solution here )

2. When I tried to add fill="var(--color-primary) to SVGs last night, the variable wasn't picked up (perhaps because these SVGs are used as background images rather than inline. -- you need to call the variables source in SVG file mention in comment #25

<link xmlns="http://www.w3.org/1999/xhtml" rel="stylesheet" href="../../css/variables.css" type="text/css"/>

you need to call the variables source in SVG file mention in comment #25

I don't want to do that. if we do that, then we might as well just hardcode the variables like my latest commit does. Or else we are declaring a variable definition repeatedly, and using it just once in each file.

Again, let's get what we have merged as it's a great improvement, and let's talk about the SVGs in a follow up issue if we need to.

Opened 📌 Discuss SVG for Umami Theme Needs work could use more details though.

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.

I adjusted the MR to align with the recent updates from version 10.1.x. Since there's a dedicated ticket for managing SVG colors, we should address it there to allow the main ticket to proceed with merging.
Given our theme's transition to an SDC structure, it's crucial to accelerate the migration to CSS variables to minimize potential conflicts down the line. Let's update the status to "needs review" to facilitate this process.

@smustgrave, I'm uncertain whether we should initiate an 11.x branch

Answer is yes. Code has to go into 11.x first and can be backported.