argyleink / open-props

CSS custom properties to help accelerate adaptive and consistent design.
https://open-props.style
MIT License
4.68k stars 191 forks source link

Shadows can be too dark #249

Open argyleink opened 2 years ago

argyleink commented 2 years ago

If someone is using the shadow props and NOT building a dark theme while the user is in a prefers dark mode, the shadows adapt to the preference and are overly dark in the light document.

I think we'd need a media query for color-scheme in order to properly handle it. then the shadows could pivot on that instead of the media query.

syndicatefx commented 2 years ago

Hi,

This together with sorting out #273 would be really great! 👍

I heve been overriding this issue using !important, when not building a dark theme:

 @media (--OSdark) {
  :where(html) {
    --shadow-color: 220 3% 15% !important;
    --shadow-strength: 1% !important;
  }

Since we are on this subject, and i know this is probably asking for to much and out of scope, it would be really exciting if we could locally scope --shadow-color and --shadow-strength. Making it easier to override shadows for those use cases where the shadow looks prettier when its hue matches the background-color it is sitting on. Tailwind provides a way to do this, in their own way.

Example:

I'm building a demo page using jit-open-props setup, i have my own normalize thing going and not using OP normalize or providing dark/light themes. The body background-color uses --orange-0, i have these square boxes each wrapping icons, these boxes have a box-shadow: var(--shadow-4). It looks ok, but if i use the same hue value from --orange-0, tweak the saturation + lightness + strength, it looks much better.

Demo

A solution could be to use --shadow-color and --shadow-strength as default/fallback values and provide an override prop, something like --shadow-color-override.

--shadow-1: 0 1px 2px -1px hsl(var(--shadow-color-override, var(--shadow-color)) / calc(var(--shadow-strength-overrride, var(--shadow-strength)) + 9%));

Just my 2 cents. Maybe i'm missing something, or over complicating things 😊

argyleink commented 2 years ago

Hi! 🙂

For shadow values overrides, removing the :where() part of the selector will give yours specificity, making it override:

@media (prefers-color-scheme: dark) {
  html {
    --shadow-color: 33.6 90% 60%;
    --shadow-strength: 8%;
  }
}

I think that helps with the 1st part? 👍🏻

273 will get some love soon 🙂 until then, here's how I'd force light theme only

i'll explore the default values strategy! i'll make a fork, thx for the reminder the default values strategy resolved the "use time" vs "parse time" issues 👍🏻 til then..

I just updated the demo to show how i'd approach colorful shadows today. a bit of duplication, but it lets you unlock it now and forget about it. i also imported the hsl values for some colors, makes it fun to pass colors to the custom shadows 🙂 thoughts?

syndicatefx commented 2 years ago

Hi!

Yes, removing the :where() should work, but in my case using jit-open-props it compiles the shadow vars into :root at the bottom of the cascade.

@media (prefers-color-scheme: dark) {
  :root {
    --shadow-color: 220 40% 2%;
    --shadow-strength: 25%;
  }
}

I don't know if this is specific to the jit setup. I am still grasping the whole OP code base, so i would have to keep !importanton my override.

Your approach in the demo is great, it also fixes the above !important issue for me, but yes a bit of duplication. Completely forgot about --[color]-hsl, very interesting.

Thanks for the light-theme only workaround, i keep forgetting we can pick and choose these bits of already available code to setup things differently.

I like that we can mix/match things around, even if it creates a bit of duplication. I think for things like colors, box-shadows and maybe fluid-typography there is always a need to override the defaults for project specific needs if using OP as-is.

Using default/fallback values is a strategy, but using it is not so straight forward since everything is declared inside :root or :where(html), it's already evaluated for all the DOM, am i right?

Unless we break up/abstract these specific props into just their <length> values(in case of box-shadows) and construct the rest of the declaration manually in our CSS, but i think it might be a bit redundant and complex compared to duplicating the whole prop like we can now, and forget about it.

Do your magic!

argyleink commented 2 years ago

I'm working out some more changes to normalize and will come back to this issue once that's shipped 👍🏻