getkirby / kirby

Kirby's core application folder
https://getkirby.com
Other
1.32k stars 168 forks source link

Blueprint: image back option with html color names that start with 'light' don't apply #4073

Closed cgundermann closed 2 years ago

cgundermann commented 2 years ago

I just noticed, that the back option of the image prop doesn't apply when I add a html color name that starts with 'light', e.g. 'lightblue', 'lightseagreen' etc... It just renders white.

mypages:
  type: pages
  layout: cardlets
  image: icon
    icon: check
    color: "#fff"
    back: "lightseagreen"

Kirby Version
3.6.1.1

distantnative commented 2 years ago

It's probably less about the light and more the color names being in there and how we currently detect if the value is supposed a shorthand for Kirby's custom CSS properties: https://github.com/getkirby/kirby/blob/main/panel/src/helpers/color.js#L12

lukasbestle commented 2 years ago

Possible solution

...that only matches actual Kirby color values:

if (
  string.match(/^(backdrop|black|dark|light|white)$/) !== null ||
  string.match(
    /^(gray|red|orange|yellow|green|aqua|blue|purple)(-[1-9]00)?$/
  ) !== null
) {
  return `var(--color-${string})`;
}

Anticipated side effects

distantnative commented 2 years ago

@lukasbestle I think the proposed solution is exactly what we should do.Will you create a PR?

Assigning this to a milestone and moving to Todo as this is straightforward and easy to review.

lukasbestle commented 2 years ago

@distantnative I'm still thinking about the case-insensitivity. It could be seen as a breaking change, but actually it isn't because the variables with different case have never worked in the past. The question is just if we should rather support any case and convert to lowercase in the returned string. Really not sure about this.

distantnative commented 2 years ago

I probably would do case-insensitive regex and then convert to lowercase. While ORANGE and orange might technically not be the same CSS property, in the Kirby blueprint world we have treated most things rather case insensitive and I think it's fine to do so here as well. I don't think we need to support ORANGE and orange side-by-side.

lukasbestle commented 2 years ago

Sounds good! I can create a PR, but I have many other tasks on my list before that. So if anyone gets to it before me, that's fine as well.

afbora commented 2 years ago

I'm not sure but how about more global solution with getComputedStyle() method?

const colorVariable = '--color-' + string.toLowercase();
const colorCustom = getComputedStyle(document.documentElement).getPropertyValue(colorVariable);

if (colorCustom) {
    return colorCustom; // or return colorVariable; 
}

return string.toLowercase();
lukasbestle commented 2 years ago

If that works reliably across browsers: Sure! 👍

afbora commented 2 years ago

Seems works for all browsers: https://caniuse.com/getcomputedstyle

What do you think @distantnative ?

distantnative commented 2 years ago

@afbora that sounds indeed neat and even more specific. does it work? from reading it, it sounds like it should be and also be supported widespread.

afbora commented 2 years ago

I haven't tested it yet. It was just theory. I will test it and let you know 👍

distantnative commented 2 years ago

We should only support the pattern alias still

  if (string === "pattern") {
    return `var(--color-gray-800) var(--bg-pattern)`;
  }
bastianallgeier commented 2 years ago