Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Color parameterization #16

Merged
merged 35 commits into from
Feb 5, 2023
Merged

Color parameterization #16

merged 35 commits into from
Feb 5, 2023

Conversation

matheus23
Copy link
Owner

Previously: #13

@nidico
Copy link
Collaborator

nidico commented Nov 4, 2022

This looks really promising!

Exposing the relevant raw parts (such as colors) of the tailwind config in a Theme module directly (and not only as Css.Style) makes a lot of sense - not only for optimization reasons.

One thing bothers me: Tailwind.Theme.Color isn't really nice to work with, I'd typically want to use something like elm-color Color instead.

As Color is opaque, it isn't possible to access the raw values from outside, so I'd suggest to add a function which converts the internal Color representation into something like NiceColor (or Maybe NiceColor? If we generated it, can the Maybe be avoided?)

type NiceColor
    = HslaSpace Float Float Float Float
    | RgbaSpace Float Float Float Float

color : Color -> NiceColor
...

which can then be transformed further, e.g. with elm-color/Color.fromRgba / .fromHsla.

Maybe something like this wouldn't be needed if Tailwind.Theme.Color wasn't opaque, but this feels a bit raw.

Alternatively, the internal representation could be made more type safe (i.e. use float instead of string and mode as a custom type), but this might be slower for internal purposes (i.e. toProperty). Also I don't fully grasp ViaVariable (is it "no opacity set"?) and Keyword (inherent and current seem pretty internal for use in Utilities to me).

Alternatively, make avh4/elm-color a dependency and add a function which turns a Tailwind.Theme.Color into an elm-color/Color.

What do you think? Do you have other ideas?

@dillonkearns
Copy link
Collaborator

Hello, good discussion @nidico! @matheus23 and I discussed exactly that in our pairing session when we first built the Theme module.

The main challenges are:

  • The colors that Tailwind uses don't map directly to elm-color's Color type (or elm-css's Color type, which is similar). For example, a Tailwind color can:
    • Be a CSS value such as inherit
    • Reference a CSS variable
  • The Color type's from elm-css and elm-color both use RGB under the hood, whereas you can use other color spaces besides RGB which include values that can't be represented by RGB (Safari just recently introduced a new set of color spaces for example)

So with those challenges, there's not always a neat mapping. And it presents some challenges for parsing those different formats. For example, here are some example theme values that reference CSS variables from the Tailwind docs:

module.exports = {
  theme: {
    colors: {
      // Using modern `rgb`
      primary: 'rgb(var(--color-primary) / <alpha-value>)',
      secondary: 'rgb(var(--color-secondary) / <alpha-value>)',

      // Using modern `hsl`
      primary: 'hsl(var(--color-primary) / <alpha-value>)',
      secondary: 'hsl(var(--color-secondary) / <alpha-value>)',

      // Using legacy `rgba`
      primary: 'rgba(var(--color-primary), <alpha-value>)',
      secondary: 'rgba(var(--color-secondary), <alpha-value>)',
    }
  }
}

Given that the CSS spec can change over time, I'm not sure we can necessarily have a NiceColor type that captures every possibility, since it's a living spec that can change over time (as it did recently with the new Safari color spaces).

And the user experience is a bit confusing with these different possibilities. If something uses a CSS value like inherit, or references a CSS variable, then it would become unusable as an elm-color Color, for example. So it feels like a messy abstraction overall.

I'm not sure what the best approach is given that. I suppose we could generate different types based on the actual occurrences in the project. For example:

type NiceColor
    = HslaSpace Float Float Float Float
    | RgbaSpace Float Float Float Float
    | Other String -- CSS keywords, or values that referenc CSS variables or can't be parsed into known color spaces or formats

What if the user's project only had Rgba values, and then we only generated the RgbaSpace variant (no HslaSpace or Other)? I guess a relevant question would be how common it is to reference variables or things that would put these into Other. If 99% of projects would only generate a single clean variant like RgbaSpace, then it could be a good approach.

@nidico
Copy link
Collaborator

nidico commented Nov 6, 2022

I think what you suggest is a pragmatic approach, which should work fine in most cases.

Let's look at a potential color : NiceColor -> elm/color.Color function which a consumer might write:

  • If Other isn't there, everything is fine and simple to write.
  • If Other is there, they could decide to just return whatever color in that case, e.g. transparency, and simply not use it in practice. Or even parse the String argument into something useful in that project.

Option 2 is messy, but I guess acceptable. So even if more than 1 % of the projects generated the Other constructor, it woudn't necessarily be very bad.

This is all a bit awkward indeed! If the somebody comes up with a better idea, that'd be cool, but if not, I'd propose to go with this for now.

@dillonkearns
Copy link
Collaborator

One nice thing is that we can always improve the set of things which are parsed into nicely structured data over time, so we don't necessarily have to ship something to parse all variations right away. It's nice to avoid breaking changes, but I imagine there will be a lot of edge cases to consider that will come up over time.

For example, we could even potentially parse things like the CSS value red or black into RGB values. Even though it's a CSS keyword, there's a direct translation into RGB space, but it does require custom code to handle that case. So we can handle more cases over time as they come up and maybe people can make pull requests to support more cases, seems like a good thing for community contributions and a large test suite to validate things.

@nidico
Copy link
Collaborator

nidico commented Nov 6, 2022

One nice thing is that we can always improve the set of things which are parsed into nicely structured data over time, so we don't necessarily have to ship something to parse all variations right away. It's nice to avoid breaking changes, but I imagine there will be a lot of edge cases to consider that will come up over time.

Yes, I fear so ;) (not knowing the full extent of the CSS spec) - but anyway - breaking changes aren't such a big deal thanks to our friendly elm compiler.

For example, we could even potentially parse things like the CSS value red or black into RGB values. Even though it's a CSS keyword, there's a direct translation into RGB space, but it does require custom code to handle that case. So we can handle more cases over time as they come up and maybe people can make pull requests to support more cases, seems like a good thing for community contributions and a large test suite to validate things.

Right! I first thought this would make up for a good separate package elm-tailwind-color-parser, however

  • this would only work if all variants were always generated (this could be ok if such a helper package existed),
  • also this would depend on a generated module, which is a bit awkward (I guess this wouldn't even work?!?),
  • even more considering that the generated module name can be configured from the elm-tailwind-modules CLI...

Alternatively, that parser could be put into one of the generated Tailwind modules.

What do you think?

@dillonkearns
Copy link
Collaborator

With the current setup of the code generator, this would live in TypeScript rather than Elm. That might be good in some ways because there might be some existing utilities out there to help translate color values to different formats, maybe even some that are used by Tailwind.

@matheus23 matheus23 merged commit 4716e30 into master Feb 5, 2023
@matheus23 matheus23 deleted the matheus23/parameterization branch February 5, 2023 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants