Define CSS variable naming scheme and usage

Instead of creating a centralized file that controls various properties of other files, we should use CSS variables for things that we know are going to be used in multiple places. Having almost everything in variables creates lots of overhead with little value. When designs change, it is easier to change the properties directly since you have more context around them than just the variable name.

Here's also a patch that shows in practice which variables would remain.

I struggle seeing an example where just changing the fontsize without the spacing between elements is the action someone has to do.
I'd also expect that if font sizes are changed we have a more broader overall design change.
@lauriii and myself had a quick discussion and it feels like font sizes like -s or -xs might be reused, like between textfield descriptions and image captions.

Discussed with @alexpott and @ckrina and we agreed that variables used by single component shouldn't be in the global space. They are specific to the implementation, and therefore when they are being changed, the context is usually required for making changes. I also don't think having variables used by a single component in the global scope is useful for implementing changed design requirements since they often require changes outside of the scope of the variables (markup, addition of other CSS rules, changes to properties outside the scope of variables).

I think these variables can be still defined in the local scope. However, I think this should be up to the person implementing, and we shouldn't require them as a pattern.

The previous patch removed too many variables. The form variables are used by multiple components.

Uploaded old version of the patch accidentally.

The scope is not the problem for that @todo, it's the :root variables are being stripped out on postcss compile. Have a look at css/dist/base/variables.css it only has comments and the variables.css aren't attached/referenced in the libraries so never loaded (how does this work?)

So once the postcss is fixed for the variables this CSS will work. I'm just not sure how you are dealing with the variables, are you having them replaced in postCSS or preserving them and using them as normal?

I reported the bugs we have discovered while working on this to the upstream repository to hopefully get some support from the maintainer of the upstream package.

For reference https://github.com/postcss/postcss-custom-properties/issues/173

Discussed with @huzooka and we agreed to move variables that are only used by a single component to the component itself. We will define those variables in the :root scope, so they have to still remain somewhat specific to not overlap with other components. Variables that are used by multiple components will remain in the variables.css. After we see variables that are left in the variables.css, we can review if we should rename them to not be named based on a specific component.

We also discussed the naming convention. We think moving the element first would make sense because that make skimming the variable types per element easier. However, when a variable isn't specific to an element, the variable should start with the variable type to make it easier to find the correct variable through autocomplete and skimming.

For variables that are not specific to an element, the pattern is: ([variable-type{color, font-size, size, line-height}])(-[property])

• --color-absolutezero
• --font-size-s

And for variables that are specific to an element, the pattern is: ([element])(-[property])(?--[state])(-[variable-type{color, font-size, size, line-height}]).

Examples:

• --button-bg-color
• --button--focus-bg-color
• --button-primary-bg-color
• --button-primary--focus-bg-color
• --button-border-radius-size

I can help on this one.

Updated the docs according to the naming convention above and will provide a patch for this later this day.

Changes since the last patch:
- move variables specific to one element to element file
Soon to come:
- rename variables based on new naming convention

I'm on changing the css variable names to the new naming convention and stumbled over such cases, where the variable has a modifier.

  --dropbutton-spacing-size--extrasmall: 0.375rem;
  --dropbutton-font-size--extrasmall: var(--font-size-xs);
  --dropbutton-line-height--extrasmall: 0.75rem;

That's not explicitly covert by our new convention as we only cover various element states with a modifier. I would suggest leaving them as they are as this modifier clearly gets to the point. Variables with state definition would then look like (based on the new convention) dropbutton--focus-spacing--large. Discussed with @lauriii and he agreed on the proposal. If there is nothing against this I would go forward like this.

It's a really big commit of 21 files but I tried to keep the overhead as low as possible. Also tested and lint this code on my site to verify it's working. With this patch we establish the new css naming convention for Claro.

There are a few special cases (like 5 over a hundred variables) which doesn't fit the naming convention – I applied the most logical change of our naming convention to them. Should not be a showstopper.

Happy reviewing! :)

Changes since the last patch:
- change variables names based on new css naming convention

I made some minor improvements to the CSS variables. I had some second thoughts on #19. As a result, I moved the variations to be as part of the element name since it's more discoverable. Any thoughts on this?

@lauriii Works for me! Indeed a good improvement, as it makes more sense to do
element-variation-property and not
element-property-variation.
One thing: We have to be careful by commiting this because they may be variables changed/added in the meantime since I changed the variables a few days ago. For example, if anyone has used the old variables in the meantime to define somewhere something. We can look in the previous commits to identify those and change them accordingly.

1. +++ b/css/src/base/variables.css
   @@ -71,152 +71,71 @@
   +  --button-default-fg-color: var(--color-text);
   +  --button-default-bg-color: var(--color-lightgray);
   +  --button-default--hover-bg-color: var(--color-lightgray-hover);
   +  --button-default--active-bg-color: var(--color-lightgray-active);
   +  --button-default--focus-bg-color: var(--button-default-bg-color);
   +  --button-default--disabled-bg-color: #ebebed;
   +  --button-default--disabled-fg-color: var(--color-grayblue);
   

   Let's change these from --button-default-[something] to --button-[something].

2. +++ b/css/src/base/variables.css
   @@ -71,152 +71,71 @@
   +  --details-desktop-wrapper-padding-bottom: calc(var(--space-m) + var(--space-s));
   

   This variable is unused.

3. +++ b/css/src/base/variables.css
   @@ -71,152 +71,71 @@
   +  --details-desktop-wrapper-padding-left: calc(var(--space-m) + var(--space-s) + var(--space-xs));
   

   I would change this from --details-desktop-wrapper-padding-left to --details-padding-start. This variable is used for the summary element right from mobile and on RTL this determines the right padding, not the left.

4. +++ b/css/src/base/variables.css
   @@ -71,152 +71,71 @@
   +  --button-primary-fg-color: var(--color-white);
   +  --button-primary-bg-color: var(--color-absolutezero);
   +  --button-primary--hover-bg-color: var(--color-absolutezero-hover);
   +  --button-primary--active-bg-color: var(--color-absolutezero-active);
   +  --button-primary--focus-bg-color: var(--button-primary-bg-color);
   +  --button-primary--disabled-bg-color: var(--color-lightgray);
   +  --button-primary--disabled-fg-color: var(--color-oldsilver);
   +  --button-danger-fg-color: var(--color-white);
   +  --button-danger-bg-color: var(--color-maximumred);
   +  --button-danger--hover-bg-color: var(--color-maximumred-hover);
   +  --button-danger--active-bg-color: var(--color-maximumred-active);
   +  --button-danger--focus-bg-color: var(--button-danger-bg-color);
   

   Well, I just noticed that we have to clarify what's considered as an element or an element with modifier.

   For example here the primary or danger variables are the modifier of the original element and not a standalone element.

   So based on the standard, these kind of variables should be changed (here: from --button-primary--focus-bg-color to --button--focus-bg-color--primary).

   An another good example are the variables of the progress (bar). For example --progress-bar-small-size-radius should be --progress-bar-size-radius--small.

Based on the last point I suggest to use double dashes for separating the element name from the other variable name components.

@huzooka, added a patch to address the points.

While working, found a couple of variables like --button--focus-bg-color & --button--focus-bg-color--danger are also not used anywhere. Shouldn't these be removed as well?

1. --- a/css/src/base/variables.css
   +++ b/css/src/base/variables.css
   @@ -57,7 +57,7 @@
      /**
       * Spaces.
       */
   -  --space-xl: 3rem; /* 4 * 16px = 48px */
   +  --space-xl: 3rem; /* 3 * 16px = 48px */
   

   👍

2. -  --details-desktop-wrapper-padding-bottom: calc(var(--space-m) + var(--space-s));
   -  --details-desktop-wrapper-padding-left: calc(var(--space-m) + var(--space-s) + var(--space-xs));
   +  --details-desktop-wrapper-padding-start: calc(var(--space-m) + var(--space-s) + var(--space-xs));
   

   +  --button--focus-bg-color: var(--button-bg-color);
   

   Do we want to remove unused variables or not? On the one hand you removed them but still left some of them? :)

3. We have to adjust the Docs accordingly if we leave the naming suggested in #24

@huzooka In my opinion, the naming --button--active-bg-color--primary isn't the best choice. [element--state-property--elementmodification] looks not that good to me. I would go for [element--elementmodification-property--state]. Basically switching them ("elementmodifier" and "state") like they are now. But yeah, this isn't a showstopper.

I removed the unused variables and fixed two.
@imalabya Please make sure to only edit css variables with search and replace (project wide) and not by hand to not miss some of them.

I'm reviewing this.

Patch #28 seems to be OK!

We should also check the docs if the current naming is up to date with the docs and vice versa.

@fhaeberle thanks for the feedback.

Kept the remaining unused variable because wasn't sure whether to keep them or not. So, just removed for feedback.

@imalabya Please make sure to only edit css variables with search and replace (project wide) and not by hand to not miss some of them.

Thanks, will do it for future reference.

Thank you everyone!