avh4 / elm-color

Standard representation of colors, encouraging sharing between packages. (This replaces elm-lang/core#Color from Elm 0.18.)
BSD 3-Clause "New" or "Revised" License
23 stars 6 forks source link

Issues with hue argument in Color.hsla #19

Open duncanmalashock opened 5 years ago

duncanmalashock commented 5 years ago

Currently the hue argument to hsla is of type Float, and expects percentage values from 0.0 to 1.0.

There are a few issues I see here:

  1. Hue is often thought of as an angular measurement (i.e. around the color wheel), so I think specifying 0.0—1.0 will lead to confusion as it has on my team.
  2. There is no way to ensure the 0.0—1.0 range with type-safety, and the behavior is undefined when hue is outside of this range.
  3. In addition to values outside of 0.0—1.0, the Float type can represent NaN and +/- Infinity values, so hsla can fail, yet it always returns a Color.

Possible solutions: (1) Use an opaque type for the hue input, that could allow the client to specify whether they're thinking of hue in terms of percentage, degrees, or radians, for example. (2) Apply modular arithmetic to the hue input to allow it to wrap values around its range. (3) Have hsla return a Result String Color instead of a Color.

mattdb commented 5 years ago

I think issue 1. is especially non-intuitive -- the CSS HSL spec uses angle in degrees (though it omits the unit, which I suppose brings us to situations such as this...).

I'm not aware of other paradigms where hue is specified from 0-1, but I'm also not a designer, so I'm far from authoritative on that.

Solution (1): Even though an opaque type would add some verbosity, it seems like it would be worth it to be more explicit and clear about what unit you're specifying.

Solution (2): This makes sense as a strategy that could help the sla values -- values less than 0 could be set to 0, and values greater than 1 set to 1 -- but I'm not sure that whatever result you'd get from mod-ing the hue value would make any more sense than just letting the conversion calculation fly with something wildly out of range...

Solution (3): I think a noticeable percentage (majority?) of users may just be hard-coding colors in Elm, and dealing with Results in that situation could make the code less clear due to extra verbosity. It may be nice to have both checked and non-checked constructors to allow for both static and interactive uses of the library.

ianmackenzie commented 5 years ago

Just ran into this myself when updating code from Elm 0.18 (where the core Color module took hue in radians) to use avh4/elm-color. I personally think that the right approach is what the old Color module did: use the standard Elm representation of angles, which is radians (easy enough to construct using the degrees function, or turns if you want to easily migrate from code that does use 0..1 values).

If hue was specified in radians then I also think it makes sense to internally use some sort of modular arithmetic to map any given angle to the 0..360 degree range, since

degrees 120
degrees -240
degrees 480

really are conceptually the same thing (at least when talking about absolute angles like hue - maybe not if you were talking about an animation!)

I don't think it's worth worrying too much about NaN or infinite values - maybe make a note in the docs about how those are treated (treat as zero? always result in black or transparent if hue is invalid?) but it's not like things like

Svg.Attributes.width (String.fromFloat width)

return a Result String (Svg.Attribute msg).

avh4 commented 4 years ago

Thanks for starting the discussion, and sorry for the slow reply.

As a reminder, the core goal of this package is to define a Color type that other packages can use to share colors with one another. With that in mind, I don't want to make too many choices about what the API for different color spaces should look like, especially in cases where there are opposing opinions about what the best API is.

My recommendation at the moment for teams that often use HSLA colors and have certain expectations about what the API should look like would be to make a package with the API you want and have it produce and consume Color values so that it's interoperable with any other packages the produce and consume Colors.

Based on the discussion here, I do agree that I think it makes sense for out-of-range hue values to be be wrapped around to give a value color. If anyone wants to make a PR for that, I'd be ready to merge it.

And I do agree that in retrospect the 0..1 range is a bit weird, and maybe it should have been done in radians to use standard Elm angles. I'm a bit hesitant to make that change though because Elm would only consider that a PATCH release, when it's actually a significant breaking change.

Also, given that it's been several months since this discussion started, I'm curious to hear the latest thoughts from folks who've been dealing with this over the past several months.