Problem/Motivation

Since we don't need to support ie11 any more, let's start moving the Umami theme to a more modern CSS base.
I forsee lots of heartache with conflics and re-rolls, if we try to tackle this issue while we are also tacking #2936875: Umami Theme - follow-up - remove elements, use classes in CSS, so let's get that committed first.

Steps to reproduce

None

Proposed resolution

Minimum: Rewrite the Umami theme colours to use css variables
Nice to have: do the same for paddings/margins/etc

Remaining tasks

Do it

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

Umami theme is now using CSS variables instead of hardcoded values for many of its CSS properties.

CommentFileSizeAuthor
#31 3347693-nr-bot.txt90 bytesneeds-review-queue-bot
#25 svg class.png293.36 KBHarish1688
#23 svg soultion 2 .png368.86 KBHarish1688
#20 svg 2.png364.19 KBHarish1688
#20 svg 1.png283.57 KBHarish1688
#17 Capture d’écran du 2023-04-12 09-51-56.png17.66 KBnod_

Issue fork drupal-3347693

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

markconroy created an issue. See original summary.

markconroy opened merge request !3756

markconroy’s picture

markconroy commented
Status: Active » Needs review

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.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

Build error

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

markconroy’s picture

markconroy commented

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

markconroy’s picture

markconroy commented
Status: Needs work » Needs review
aziza_a’s picture

aziza_a commented
Status: Needs review » Needs work

Build error - command failed

spokje’s picture

spokje
Primary language Dutch
commented

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

markconroy’s picture

markconroy commented
Status: Needs work » Needs review

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

spokje’s picture

spokje
Primary language Dutch
commented

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

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

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

markconroy’s picture

markconroy commented
Status: Needs work » Needs review
smustgrave’s picture

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

Looks good!

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

gauravvvv’s picture

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

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); /
smustgrave’s picture

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

Thanks.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new Capture d’écran du 2023-04-12 09-51-56.png17.66 KB

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.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Issue summary: View changes
nod_’s picture

nod_
Primary language French
Location Lille
commented

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

Harish1688’s picture

Harish1688 commented
StatusFileSize
new svg 1.png283.57 KB
new svg 2.png364.19 KB

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

1. solution - working
we can redeclare the variable in the SVG file, 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="..."/>  <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)
we can call the variables css file in the SVG file, working fine. 

<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"/>
<style>

.cls-1{fill:var(--color-primary);}</style></defs><path class="cls-1" d=".."/>  <circle class="cls-1" cx="300.24" cy="339.59" r="9.2" transform="rotate(-11.9 300.247 339.58)"/></svg>
Harish1688’s picture

Harish1688 commented
Status: Needs work » Needs review
Harish1688’s picture

Harish1688 commented
Harish1688’s picture

Harish1688 commented
StatusFileSize
new svg soultion 2 .png368.86 KB

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

markconroy’s picture

markconroy commented

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.

Harish1688’s picture

Harish1688 commented
StatusFileSize
new svg class.png293.36 KB

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.

markconroy’s picture

markconroy commented

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.

Harish1688’s picture

Harish1688 commented

Hi markconroy,

1. We need to go through every CSS value in each SVG, which could be tricky -- we could do it, 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"/>

markconroy’s picture

markconroy commented

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.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Related issues: +#3357143: Discuss SVG for Umami Theme

Opened #3357143: Discuss SVG for Umami Theme could use more details though.

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.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3347693-nr-bot.txt90 bytes

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.

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

snig’s picture

snig commented
Status: Needs work » Needs review

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

smustgrave’s picture

smustgrave commented

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

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

snig opened merge request !4951

snig’s picture

snig commented
Status: Needs work » Needs review
snig’s picture

snig commented
Status: Needs review » Needs work

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.