Jieiku / abridge

Fast & Lightweight Zola Theme
https://abridge.pages.dev/
MIT License
168 stars 45 forks source link

Better dynamic color themes #124

Closed eugenesvk closed 1 year ago

eugenesvk commented 1 year ago

Fixes two issues:

  1. Orang vs blue colors are ignored because all colors /// Dark Colors and /// Light Colors are defined in one place abridge.scss This is done by adding a null check and making all centrally defined colors a null, so that if the user passes nothing in the overrides, default colors are used So now changing $color: "blue",// color template to use/override: orange, blue, blueshade changes the link colors unlike before

  2. Also fixes the issue described here https://github.com/Jieiku/abridge/pull/114#issuecomment-1615601784 where loading dynamically via a meta function is invalid per spec since then you can't use the imported module's mixins

Jieiku commented 1 year ago

When I implemented this, I tested changing individual colors, adding fonts, enabling and disabling icons, changing page width, all of which worked with zola. I think I assumed everything else would work similarly, but I just tested changing from orange to blue for the base color template and can see that it did not work.

That really is a shame about meta.load-css not working as I had hoped, because it would have kept things simple.

I definitely appreciate you pointing out this issue because I was unaware of it.

I am considering removing the individual color template files, because they not really simplifying anything if we have to use this type of logic anyway:

@mixin getlight($color,$light)
  @if      $color == "orange"
    @include    orange.light($light...)
  @else if $color == "blue"
    @include      blue.light($light...)
  @else if $color == "blueshade"
    @include blueshade.light($light...)
  @else
    @include    orange.light($light...)

The above type of logic is what I was trying to avoid when I first used meta.load-css() Instead I am thinking of defining the individual colors within the main abridge.scss file within a similar if statement.

This would keep the logic for the colors right next to the color definitions themselves, which would probably be easier for somebody new to abridge to figure out what is going on if they try adding a new set of colors. (I planned on adding atleast a few more colors myself, similar to terminimal:

eugenesvk commented 1 year ago

That really is a shame about meta.load-css not working as I had hoped, because it would have kept things simple.

Yeah, it is, it's one of the bigger limits of sass, similarly to how you can't use @use imports within conditionals. Also inability to dynamically construct var names, otherwise the null check in syntax colors could be relegated to a function

Instead I am thinking of defining the individual colors within the main abridge.scss file within a similar if statement.

The individual files might help a bit with comparison since you can open them side-by-side, and also might be a bit easier to copy&paste and change instead of having 7×2 lists in the same file, but maybe indeed it's not that big of a difference given the complexity of the selection logic

Jieiku commented 1 year ago

I just pushed a commit that should resolve this issue, I create a site that used abridge as a theme, set the theme color to blueshade and then overrided a couple colors for both the base theme and the syntax, and it all worked.

https://github.com/Jieiku/abridge/commit/a0abcac2123087be8add82e0e5eedf3af7bb5a2a

Would REALLY REALLY appreciate it if you could take a look on your end and see if you spot any outstanding issues.

Thanks again for the help!

eugenesvk commented 1 year ago

seems to be working fine, and making the null check work automatically is a nice bonus, though the colors are defined twice - in the mixin and in the function

Jieiku commented 1 year ago

Good catch, the default values in the mixin are no longer necessary, I cleaned that up just now:

https://github.com/Jieiku/abridge/commit/d367a4b79c801ac0ac084032aaee388616ffe97c