GettEngineering / Prism

Gett's Design System code generator. Use Zeplin Styleguides as your R&D's Single Source of Truth.
https://prism.tools
MIT License
361 stars 21 forks source link

That one color.a edge case #31

Closed gdelmas closed 4 years ago

gdelmas commented 4 years ago

Now that you introduced inverting conditionals, the alpha component of the color token has come to my attention. In most cases, when it is 1.0, it can be ignored. Unfortunately if it is not, one might need a different emit. For example color.rgb vs color.argb.

Do you think it would make sense to allow to use a conditional on color.a?

In one possible implementation this could be {{% IF color.a %}}. This would match all but 1.0. Which i can see, might be confusing. As the value would cast to the opposite. Alternatively maybe

The latter probably being the most complicated to implement and just opening a completely different can of worms.

Sorry for bombarding you with issues.

freak4pc commented 4 years ago

I'm not sure it would make sense to add this kind of conditional, it seems a bit niche use case, and can just be used as 1.0 instead of being ignored in most cases I'm aware of. Can you explain what's the use case here?

gdelmas commented 4 years ago

I noticed that when i was working on a CSS emit, where the norm is to use

#FFFFFF

But this is not possible with alpha, so in that case I needed to emit either

rgba(255, 255, 255, 0.5)

Or dependant of the use case:

color: #FFFFFF;
opacity: 0.5;
freak4pc commented 4 years ago

I still don't think there's anything here requiring a conditional.

When using auto-generated code (like here), you want to reach for the most common and basic use-case. So in this case, emitting rgba(r, g, b, a) makes a ton of sense and its also a widely-used standard that works on all browsers. I don't think there's anything problematic with that, even when Alpha is 1.0 - you would just have it explicitly emitted, but I don't see anything wrong with that.

By the way, for Andorid they have ARGB (where the first two digits are the alpha). I see that in CSS there's RGBA (https://drafts.csswg.org/css-color/#hex-notation, see "8 digits"), but I'm not sure if it's supported in all browsers - if yes, this might be a nice addition to have here.

WDYT ?

freak4pc commented 4 years ago

Ok, so rgba() has a really wide support (~99% of browsers):

image

Using #RGBA is quite lower but still widely supported at around 90% of browsers:

image

gdelmas commented 4 years ago

you are correct. i think rgba is probably the safer bet, and supported already.

i guess it comes down to being stylistic too. i was trying to match Zeplins emit, with the thought that it will reduce mental load, when debugging in the browser and when some Zeplin output shows the same as the generated files. This is to prevent for the same thing to be added multiple times into the codebase with different syntaxes.

This is what Zeplins CSS extension outputs:

:root {
  --blue: #0000ff;
  --yellow: #ffff00;
  --green: #00ff00;
  --black: #000000;
  --black50: rgba(0, 0, 0, 0.5);
  --white: #ffffff;
  --red: #ff0000;
}

Here is their conditional: https://github.com/zeplin/stylesheet-extensions/blob/6906b11c7e118f50f5fc41918d98099fe9ed85cd/packages/zeplin-extension-style-kit/values/color.js#L65

But as you were saying, in the end this comes down to a teams preference how they want it.


While researching their extension and wondering why they do that special emit with alpha i had to find out, that including an alpha channel might incur a performance hit on older browsers: https://stackoverflow.com/a/54018451/3650874 I was not aware of this.

freak4pc commented 4 years ago

The performance hit you're mentioning is negligible. Anyways, I don't think "style" plays a huge role in automatically-generated code since the developers aren't actually maintaining it. It's like you'l want to have styling for machine bit-code :)

Regardless, I think having == and != operator could be really nice but there's obviously some complexity that needs consideration there.

I'll close this issue and open a tracking issue for that, if that's OK with you.

Thanks!