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

Added Color from hex code, updated examples #222

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Anton-4
Copy link

@Anton-4 Anton-4 commented May 26, 2020

I think I am still going to clean up the code a bit.
Let me know if you have any suggestions @mdgriffith !

@Anton-4
Copy link
Author

Anton-4 commented May 26, 2020

I have added a function hex that returns a Result and also a function hexOrRed which defaults to the color red if the result was an error. It feels a bit dirty but I think having to deal with result handling every time you want to use a color is too inconvenient.

@Anton-4
Copy link
Author

Anton-4 commented May 30, 2020

The test suite is failing at "Run webfactory/[email protected]" with error
"##[error]The ssh-private-key argument is empty. Maybe the secret has not been configured, or you are using a wrong secret name in your workflow file.". I feel like I should not mess with this, can you take a look @mdgriffith?

src/Element.elm Outdated Show resolved Hide resolved
@philer
Copy link

philer commented Sep 26, 2020

Wouldn't it be easier to have an Int -> Color version? Something like

hex : Int -> Color
hex n =
    rgb255 (n // 0x10000) (n // 0x100 |> modBy 0x100) (modBy 0x100 n)

-- example
white : Color
white = hex 0xffffff

@Anton-4
Copy link
Author

Anton-4 commented Sep 27, 2020

Do you mean easier in use or easier in implementation?
Css and many online color tools use hashtag prefixed hex notation.
These online tools also often use click to copy.
I chose Strings for minimal friction with existing tools, making it easier to port css and familiarity.
It is also common to use for example fff as a shorthand for ffffff, so that would have to be added as well.
I also like the hexOrRed function to notify users when they made a mistake such as providing a 7-letter hex code as argument.

I'm not saying you're wrong just outlining my thought process and arguments :) .

@philer
Copy link

philer commented Sep 30, 2020

I was also thinking a little bit about performance. I haven't benchmarked any of it but I'm assuming all that string parsing can't do very well. Maybe it doesn't matter.
The 0x prefix actually feels more natural to me because it doesn't need quotes, but obviously I can't speak for everyone. Some tools I use (such as color syntax highlighting) understand the 0x notation as well.

FWIW, here are hex versions long and short without and with alpha channel. I haven't tested these extensively but I believe they are correct. The names may be argued about. :D

-- regular 0xRRGGBB
hex6 : Int -> Css.Color
hex6 n =
    rgb (n // 0x10000) (n // 0x100 |> modBy 0x100) (modBy 0x100 n)

-- short 0xRGB
hex3 : Int -> Css.Color
hex3 n =
    rgb
        (n // 0x100 * 0x11)
        ((n // 0x10 |> modBy 0x10) * 0x11)
        (modBy 0x10 n * 0x11)

-- with alpha 0xRRGGBBAA
hex8 : Int -> Css.Color
hex8 n =
    rgba
        (n // 0x1000000)
        (n // 0x10000 |> modBy 0x100)
        (n // 0x100 |> modBy 0x100)
        (toFloat (modBy 0x100 n) / 0xff)

-- short with alpha 0xRGBA
hex4 : Int -> Css.Color
hex4 n =
    rgba
        (n // 0x1000 * 0x11)
        ((n // 0x100 |> modBy 0x10) * 0x11)
        ((n // 0x10 |> modBy 0x10) * 0x11)
        (toFloat (modBy 0x10 n) / 0xf)

@Anton-4 Anton-4 marked this pull request as draft September 30, 2020 08:46
@Anton-4
Copy link
Author

Anton-4 commented Sep 30, 2020

I have converted the PR to a draft now so it does not get merged in it's current state. I suggest we let @mdgriffith decide on the use of Ints or Strings or perhaps both.

@jangerhard
Copy link

Any updates on this?

@jaladh-singhal
Copy link

Having hex colors functionality will be great - @mdgriffith @Anton-4 any updates?

@Anton-4
Copy link
Author

Anton-4 commented Oct 23, 2021

@jaladh-singhal everything is ready for @mdgriffith to decide whether he wants to use hex functions that accept Int or String or both.

@jakequade
Copy link

Would be great to have hex functionality, @mdgriffith @Anton-4

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.

6 participants