Add Color module.#448

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

cknitt merged 1 commit intomasterfromcolor

Apr 26, 2019

Merged

Add Color module.#448

cknitt merged 1 commit intomasterfromcolor

Apr 26, 2019

Conversation

Copy link

Member

cknitt commentedApr 26, 2019

@MoOx As discussed on Discord:

This replacesStyle.color withColor.t.
The latter is now not an abstract type anymore, but an alias tostring.
This way people can just pass strings to color props like they can in JS.
This makes the API easier to use, but less safe.
For mitigation, we provide helper functions and constants for safe color creation/use in theColor module.

Add Color module.

7cf63f4

cknitt requested review fromMoOx andsgny

April 26, 2019 16:45

MoOx approved these changes

Apr 26, 2019

View reviewed changes

Copy link

Member

MoOx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This approach is so cool.

cknitt merged commit3360c24 intomaster

Apr 26, 2019

Copy link

Collaborator

sgny commentedApr 26, 2019•
edited
Loading

@cknitt Sure being able to pass strings has upside in ease and downside in less safety, on the other hand, now people will not make mistakes with named colors (cornflowerblue, etc.) if they use the constants and not simply pass their names as string.

And we could always move back to an abstract type, simply by adding a type-conversion function and defining an appropriate interface for the module. So overall, I agree this is a good move.

Great work and thanks for the huge effort.