color is legacy #4
Conversation
SondraE
changed the title
There was a problem hiding this comment.
This is on the right track Sondra. The format looks good. We will have to decide what tests to include (we probably won't test all color spaces?).
I think adding some error tests would be next here (look at some of the "error/too_few_args", "error/too_many_args", "error/type" tests in the project. A lot of these follow a similar pattern.)
spec/core_functions/color/legacy.hrx
Outdated
|
|
||
| <===> | ||
| ================================================================================ | ||
| <===> srgb/input.scss |
There was a problem hiding this comment.
At some point we'll have to decide which of this need to be included to feel like we've covered enough cases. We probably won't need to test all of the color types.
Co-authored-by: david herron <41588129+dvdherron@users.noreply.github.com>
There was a problem hiding this comment.
@SondraE looks great so far. I added some suggestions as I believe commas are no longer needed in the color notations.
I don't know how many color formats we will want to test but it may make sense to include something like:
- a named color
- hex color
- maybe something with alpha in the legacy color space?
spec/core_functions/color/legacy.hrx
Outdated
| @@ -0,0 +1,182 @@ | |||
| <===> options.yml | |||
There was a problem hiding this comment.
@SondraE I don't know this for sure, but I am guessing the filename is_legacy.hrx would be preferred.
spec/core_functions/color/legacy.hrx
Outdated
| ================================================================================ | ||
| <===> hwb/input.scss | ||
| @use "sass:color"; | ||
| a {b: color.is-legacy(hwb(0, 50%, 0%))} |
There was a problem hiding this comment.
| a {b: color.is-legacy(hwb(0, 50%, 0%))} | |
| a {b: color.is-legacy(hwb(0 50% 0%))} |
I believe Sass will support with or without commas
spec/core_functions/color/legacy.hrx
Outdated
| ================================================================================ | ||
| <===> hsl/input.scss | ||
| @use "sass:color"; | ||
| a {b: color.is-legacy(hsl(110, 31%, 32%))} |
There was a problem hiding this comment.
| a {b: color.is-legacy(hsl(110, 31%, 32%))} | |
| a {b: color.is-legacy(hsl(110 31% 32%))} |
spec/core_functions/color/legacy.hrx
Outdated
| ================================================================================ | ||
| <===> srgb/input.scss | ||
| @use "sass:color"; | ||
| a {b: color.is-legacy(srgb(115, 20, 210))} |
There was a problem hiding this comment.
@mirisuzanne @dvdherron I've not really come across a lot with srgb used in a css notation like this before, but would it be more like:
a {b: color.is-legacy(color(srgb 0.45098 0.07843 0.823530))}
There was a problem hiding this comment.
https://css.oddbird.net/sass/color-spaces/proposal/#predefined-color-spaces
‘Predefined color spaces’ can be described using the color() function.
The predefined RGB spaces are:
- srgb
- srgb-linear
- display-p3
- a98-rgb
- prophoto-rgb
- rec2020
spec/core_functions/color/legacy.hrx
Outdated
| ================================================================================ | ||
| <===> srgb-linear/input.scss | ||
| @use "sass:color"; | ||
| a {b: color.is-legacy(?????)} |
There was a problem hiding this comment.
@mirisuzanne @dvdherron @SondraE and same for here?
a {b: color.is-legacy(color(srgb-linear 0.45098 0.07843 0.823530))}
spec/core_functions/color/legacy.hrx
Outdated
| ================================================================================ | ||
| <===> prophoto-rgb/input.scss | ||
| @use "sass:color"; | ||
| a {b: color.is-legacy(?????)} |
There was a problem hiding this comment.
| a {b: color.is-legacy(?????)} | |
| a {b: color.is-legacy(color(prophoto-rgb 0.42444 0.934918 0.190922))} |
spec/core_functions/color/legacy.hrx
Outdated
| ================================================================================ | ||
| <===> a98-rgb/input.scss | ||
| @use "sass:color"; | ||
| a {b: color.is-legacy(?????)} |
There was a problem hiding this comment.
| a {b: color.is-legacy(?????)} | |
| a {b: color.is-legacy(color(a98-rgb 0 1 0))} |
spec/core_functions/color/legacy.hrx
Outdated
| ================================================================================ | ||
| <===> xyz-d50/input.scss | ||
| @use "sass:color"; | ||
| a {b: color.is-legacy(?????)} |
There was a problem hiding this comment.
| a {b: color.is-legacy(?????)} | |
| a {b: color.is-legacy(color(xyz-d50 0.2005 0.14089 0.4472))} |
spec/core_functions/color/legacy.hrx
Outdated
| ================================================================================ | ||
| <===> xyz-d65/input.scss | ||
| @use "sass:color"; | ||
| a {b: color.is-legacy(?????)} |
There was a problem hiding this comment.
| a {b: color.is-legacy(?????)} | |
| a {b: color.is-legacy(color(xyz-d65 0.21661 0.14602 0.59452))} |
spec/core_functions/color/legacy.hrx
Outdated
| ================================================================================ | ||
| <===> oklab/input.scss | ||
| @use "sass:color"; | ||
| a {b: color.is-legacy(oklab(0.44027, 0.08818, -0.13386))} |
There was a problem hiding this comment.
| a {b: color.is-legacy(oklab(0.44027, 0.08818, -0.13386))} | |
| a {b: color.is-legacy(oklab(0.44027 0.08818 -0.13386))} |
I believe the commas would be an error for oklab, oklch,lab, and lch
… hex, name, and alpha colors
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ invocation | ||
| ' | ||
| ,--> sass:color | ||
| 1 | @function color.is-legacy(rgb($amount $amount $amount)) { |
There was a problem hiding this comment.
I think instead of testing how the is-legacy() function works this is instead testing if the correct values are in the rgb() function. For is-legacy(), the "too few" error should be triggered with an empty function (color.is-legacy()) since this function only takes one argument ($color).
And then for all of the errors in this file, see Natalie's note about leaving the error message content blank.
| <===> | ||
| ================================================================================ | ||
| <===> error/too_many_args/input.scss | ||
| a {b: color.is-legacy(rgb(0 255 0 15))} |
There was a problem hiding this comment.
Same here. We're just trying to test is-legacy(). A "too many" args error should be triggered if we run this function with more than one color:
a {b: color.is-legacy(rgb(0 255 0), blue)}
(see the functions for blue() and blackness() for examples of errors for a single argument function)
| input.scss 1:7 root stylesheet | ||
|
|
||
| <===> error/too_many_args/error-libsass | ||
| Error: wrong number of arguments (4 for 3) for `color.is-legacy' |
There was a problem hiding this comment.
Since this probably won't ever be implemented in libsass, we can omit any libsass error messages for this function and any entirely new function we write tests for. At the top of the file we have told the test runner to ignore libsass completely with :ignore_for: - libsass
| <===> | ||
| ================================================================================ | ||
| <===> error/type/input.scss | ||
| a {b: color.is-legacy(rgb(0 255 red))} |
There was a problem hiding this comment.
| a {b: color.is-legacy(rgb(0 255 red))} | |
| a {b: color.is-legacy(1)} |
This function expects a color. A number would be the wrong type for the $color argument.
It might be good to also write an additional type test (error/type/string/input.scss) to make sure this errors if $color is a string (in quotes):
a {b: color.is-legacy("blue")}
|
|
||
| <===> | ||
| ================================================================================ | ||
| <===> lch/input.scss |
There was a problem hiding this comment.
| <===> lch/input.scss | |
| <===> named/input.scss |
We could swap out one of the tests or add an additonal test called named. This will test that the function also works if the argument is explicitly named e.g. $color: lch(90.6 52.8 197)
| ================================================================================ | ||
| <===> lch/input.scss | ||
| @use "sass:color"; | ||
| a {b: color.is-legacy(lch(90.6 52.8 197))} |
There was a problem hiding this comment.
| a {b: color.is-legacy(lch(90.6 52.8 197))} | |
| a {b: color.is-legacy($color: lch(90.6 52.8 197))} |
There was a problem hiding this comment.
@SondraE These updates look good to me. I think this is ready to move forward....which I think means opening a PR on the sass/sass-spec repo, right @dvdherron? We are go over that tomorrow if so if you want.
Missed this but it looks like PR was opened successfully? :) 🚀 |
This updates most tests for the currently-implemented chunk of the Color 4 feature. A few tests that need more thorough reworking (such as being moved out of `error/` for things that are no longer intended to be errors) are marked as "todo" instead. See sass#1828 See sass/sass#2831
This also skips a few color tests for the embedded host
Description
(sass#1842)
Steps to test/reproduce