lemol / ant-design-icons-elm

⭐ Ant Design Icons for Elm
https://package.elm-lang.org/packages/lemol/ant-design-icons-elm/latest/
MIT License
5 stars 0 forks source link

Cannot control color of TwoTone Icons #1

Open tgelu opened 4 years ago

tgelu commented 4 years ago

With outlined icons a fill: currentColor on the svg works just fine to control the color of the icon. But for two-tone icons I can't seem to find a way since the fill values seem to be hardcoded.

Would it be reasonable to accept a Color value for two tone icons?

lemol commented 4 years ago

@tgelu sure!

Maybe a syntax like:

accountBookTwoTone : List (Attribute msg) -> { color : String } -> Html msg
lemol commented 4 years ago

hmm, actually we need both the primary and the secondary color, like:

accountBookTwoTone : List (Attribute msg) -> { primaryColor : String, secondaryColor : String } -> Html msg
lemol commented 4 years ago

Definitely they are optional:

accountBookTwoTone : List (Attribute msg) -> { primaryColor : Maybe String, secondaryColor : Maybe String } -> Html msg

Check: https://ant-design-icons-elm-git-feat-twotone-colors.lemol.vercel.app/#Default/Application%20Icons/Two%20Tones

What do you think?

tgelu commented 4 years ago

Thanks for the reply!

I think I have 2 short thoughts.

1) I am not sure what it would mean to provide Nothing as primaryColor for example, or to provide Nothing for both. If it means something specifically (like the color defaulting to transparent or white) I would think it's a better interface to make that explicit. That can be done either by accepting some RGBA color which can express transparency or, for example, to expect a custom type like:

type Color
    = Transparent
    | SolidColor RGBValue

2) But I am wondering if there's an optimal relation between the primary and secondary colors which is desirable when using two tone icons. If there is, it would be nice if these icons would accept simply one color and deduce the secondary color from that (by increasing lightness by a certain amount to create contrast).

If you think option 2 is not ideal or too restricting I still think it would be better to have both primary and secondary colors as required, and if transparency is one of the possible accepted colors then it is better addressed either with accepting a RGBA value or a custom type like in option 1, as opposed tom Maybe's that allow vague input.

What do you think?

lemol commented 4 years ago

@tgelu thank you for yours thoughts,

  1. I was trying to follow the API from ant-design icon for react, so that primaryColor = Nothing means the default primary color, which is #333 (default secondary color is #E6E6E6 ). I agree that it is not good being implicit, although I am not sure about creating a custom Color type, since other colors from Html.Attributes are represented as String.

  2. Yes we can implement a relation between the primary and secondary colors. Actually the react implementation doesn't receives the secondary color, but calculates it from the primary. I don't know if we can easily calculate the color using string hex colors. If we use the custom Color type as you suggest then it maybe easiest to compute the secondary color from the primary one.

lemol commented 4 years ago

Thinking here...

To remove the Maybe and make it explicit we can define a different function for the default case:

accountBookTwoTone : List (Attribute msg) -> { primaryColor : String, secondaryColor : String } -> Html msg

-- HERE { primaryColor = "#333", secondaryColor = "#E6E6E6" }
accountBookTwoToneDefault : List (Attribute msg) -> Html msg
tgelu commented 4 years ago

Regarding the custom color I suggested, I was actually thinking about using https://package.elm-lang.org/packages/avh4/elm-color/latest/ which I have seen other color-related packages use. It has a type Color intended as a type to be shared between packages. There is also this additional package for more functionality on that type https://package.elm-lang.org/packages/noahzgordon/elm-color-extra/latest/

I personally think that accepting one Color type from avh4/elm-color is ideal. You can easily calculate the coresponding secondary color with https://package.elm-lang.org/packages/noahzgordon/elm-color-extra/latest/Color-Manipulate