johannesjo / angular-material-css-vars

Little library to use css variables with @angular/material
https://johannesjo.github.io/angular-material-css-vars/
MIT License
194 stars 32 forks source link

mixin `mat-css-set-palette-defaults` not generating any code #158

Open yutamago opened 1 year ago

yutamago commented 1 year ago

:ghost: Brief Description

I'm not sure if I'm missing something, but the mixin mat-css-set-palette-defaults as described in the readme is not generating any code:

Input:

/**
 MATERIAL CSS DEFAULTS
 */
@include mat-css-vars.mat-css-set-palette-defaults($md-primary, "primary");
@include mat-css-vars.mat-css-set-palette-defaults($md-accent, "accent");
@include mat-css-vars.mat-css-set-palette-defaults($md-warn, "warn");

Output:

/**
 MATERIAL CSS DEFAULTS
 */

The result is, that it's not using my defined palette per default. It uses some palette but I can't tell which one on first glance.

:pancakes: Affected version

5.0.2 (probably still exists in 6.0.1 since the mixin didn't change)

:police_car: Expected behavior

The mat-css-set-palette-defaults mixin should generate default css variables with the given palette.

:heavy_plus_sign: Additional context

Looking at the current implementation, I'm wondering how this should work, since the $new-map is never actually written to the file: https://github.com/johannesjo/angular-material-css-vars/blob/c070b329b1b0e1104fc7bf398b3025be17d78086/projects/material-css-vars/src/lib/_public-util.scss#L176

json-derulo commented 1 year ago

Thanks for reporting the issue. From a quick look it seems like #72 broke this functionality, more spceifically this commit: https://github.com/johannesjo/angular-material-css-vars/commit/c21082a2b5f3d773022e33be0e84c57ec125f253

I will need to investigate a bit more when I have the time, thank you for your understanding and patience.

yutamago commented 1 year ago

I found a solution. Can post it tomorrow when I'm back at my computer.

yutamago commented 1 year ago

This is my modified version of the mixin:

@mixin mat-css-set-palette-defaults($css-var-map, $paletteType: "primary") {
  @each $var, $defaultVal in $css-var-map {
    @if ($var != "contrast") {
      $colorVal: hex-to-rgb($defaultVal);
      @if $colorVal != null {
        --palette-#{$paletteType}-#{$var}: rgb(#{$colorVal});
      }
    } @else {
      @include mat-css-set-palette-defaults($defaultVal, "#{$paletteType}-contrast");
    }
  }
}

Unlike the example in the readme, you have to use it within a selector, i.e. :root

:root {
    @include mat-css-vars.mat-css-set-palette-defaults($md-primary, "primary");
    @include mat-css-vars.mat-css-set-palette-defaults($md-accent, "accent");
    @include mat-css-vars.mat-css-set-palette-defaults($md-warn, "warn");
}

^ This is because the modified mixin is calling itself recursively. You could avoid this very easily, for example by providing another param like $wrapWithRoot: true and pass false in the recursion.

json-derulo commented 1 year ago

Thanks for figuring out a workaround. Question is how can it be fixed, preferably in a "backwards compatible" (non-breaking) way.

It could be done by creating a new mixin with the code you posted. The old broken mixin could be deprecated. Would you be willing to create a pull request?

yutamago commented 1 year ago

Sure, I can submit a PR on Monday or Tuesday