-
-
Notifications
You must be signed in to change notification settings - Fork 33
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
color-mix function #408
color-mix function #408
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
6376716
to
5f58488
Compare
e98c793
to
d7ac7ad
Compare
13f6e19
to
5ed734d
Compare
[%expr [%css "background-color: color-mix(in srgb-linear, white, red)"]], | ||
[%expr | ||
CssJs.backgroundColor( | ||
`colorMix2(( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know I proposed colorMix2/colorMix4, but in reality what makes a bit more sense is to keep colorMix as a single arity but then the first argument of the variant is in1/
in2/`in3, does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean more or less like this?
`colorMix((
`in1(`in_, `srgb),
(CssJs.white, `percent(10.)),
(CssJs.red, `percent(90.)),
))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if that's the case, method<>
sounds better as it portrays the name of the argument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More like:
`colorMix((
`in1(`srgb),
(CssJs.white, `percent(10.)),
(CssJs.red, `percent(90.)),
))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
26135b6
to
467f391
Compare
@@ -4006,6 +4110,8 @@ let parse_declarations = (~loc: Location.t, property, value) => { | |||
| Error(_) => | |||
switch (render_to_expr(~loc, property, value)) { | |||
| Ok(value) => Ok(value) | |||
| exception (InvalidValue(v)) => | |||
Error(`Invalid_value(value ++ ". " ++ v)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the .
is necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and I don't see the \
Invalid_value` error handled anywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The .
separates the property value from the error message. Which is rendered like this
Error: Property 'background-color' has an invalid value: 'color-mix(srgb,
white 0%, red 0%). Both percentages can not be 0!'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm! The error message appears inside quotes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're calling Invalid_value here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to keep it as it is 👍🏼
@@ -24,6 +24,8 @@ let (let.ok) = Result.bind; | |||
/* TODO: Add payload on those exceptions */ | |||
exception Unsupported_feature; | |||
|
|||
exception InvalidValue(string); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep all exceptions with the same casing
s/InvalidValue/Invalid_value
" 'in' && [<rectangular-color-space> | <polar-color-space> <hue-interpolation-method>?] " | ||
] | ||
and function_color_mix = [%value.rec | ||
// TODO: Use <extended-percentage> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this todo in the scope of the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm!
Let's do it in another PR, what's your take?
No description provided.