terezka/line-charts

Color type

Closed this issue · 5 comments

2mol commented

Hi! I loved following the progress and design ideas behind elm-plot (and I guess now lines) and I was wondering what your opinions are on using the core Elm Color type in the future.

I see your Color is currently a String alias. I would say there is a good argument to be made for leveraging a proper RGB tuple type, as well as the helper functions that come with that. Would you be open to that?

I just finished porting some very nice colormaps from matplotlib as an Elm library (https://github.com/2mol/elm-colormaps), and I think they would go very well with the opinionated curating approach that you take towards the choice of colors.

I volunteer to help with the work of course.

@2mol Seems like a natural choice.

2mol commented

Turns out that the Svg library expects String for Attributes.stroke and the like. Replacing the type itself really doesn't need all that many changes. The choice is between

  1. pulling eskimoblood/elm-color-extra as an additional dependency (just for colorToHex)
  2. waiting for elm-svg to maybe use the core Color type
  3. waiting for core to provide an equivalent to colorToHex

Did I miss an option? I think neither options are great, but maybe leaving it as-is and waiting for Color to be used in svg makes the most sense?

Hi! Thanks for your initiative! I already made a note about this change, but it will probably be the done in a later release, as it is not blocking. However, if you want to come up with some suggestions for this, you're more than welcome :)

2mol commented

Cool. Since I think that it's better to sometimes copy a little bit of code, my suggestion is a colorToHex helper function for somewhere like Internals/Util.elm. I went ahead and wrote a simple implementation.

I am aware that in addition to elm-color-extra, there is also rtfeldman/hex, BUT with the assumption that color channels are always in 0-255, the helper functions are fairly simple.

So I would like to make a case to use the code below, maybe with the goal of getting a cleaner version of it into core Color.

If people are ok with this, then I can weave it in and send a PR.

(sorry if it's bad form to paste a big chunk of code, but at least it's REPL-testable)

module ColorUtil exposing (colorToHex)

import Color exposing (Color)
import Dict exposing (Dict)


colorToHex : Color -> String
colorToHex c =
    let
        { red, green, blue } =
            Color.toRgb c
    in
        [ red, green, blue ]
            |> List.map toHex
            |> String.concat


hexDict : Dict Int String
hexDict =
    [ "0", "1", "2", "3", "4", "5", "6", "7", "8", "9", "a", "b", "c", "d", "e", "f" ]
        |> List.indexedMap (,)
        |> Dict.fromList


toHex : Int -> String
toHex n =
    let
        digit1 =
            n // 16

        digit0 =
            if digit1 /= 0 then
                n % (digit1 * 16)
            else
                n

        ( hex1, hex0 ) =
            ( Dict.get digit1 hexDict
            , Dict.get digit0 hexDict
            )
    in
        case ( hex1, hex0 ) of
            ( Just a, Just b ) ->
                a ++ b

            _ ->
                "00"

Already implemented now!