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

Color specs #1524

Closed
wants to merge 2 commits into from
Closed

Color specs #1524

wants to merge 2 commits into from

Conversation

Awjin
Copy link
Contributor

@Awjin Awjin commented Mar 3, 2020

See #1516

Adds specs for the value color.

@Awjin Awjin requested a review from nex3 March 3, 2020 00:23
@Awjin Awjin changed the base branch from restructure-directories to master March 3, 2020 00:31
Comment on lines +2 to +3
$left: #000;
$right: true;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need variables here; I think you can just include these inline.

<===> null/options.yml
---
:todo:
- sass/libsass#2413
Copy link
Contributor

Choose a reason for hiding this comment

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

I think Dart Sass is actually incorrect here. Ruby Sass also produced an error for color + null, and that's not the sort of thing that's particularly useful to change in a new implementation. Same goes for other null operations.

<==============================================================================>
<===> error/map/input.scss
$left: #000;
$right: ("key": 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$right: ("key": 0);
$right: (1: 0);

Alternately, per this style guide rule this could have a string name but it would have to be a single letter.

@@ -0,0 +1,93 @@
<===> boolean/true/input.scss
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess there's not much harm in having all of these test cases explicitly tested, but I think it's probably overkill when the underlying behavior is that and with a color LHS should always return the RHS. Same for the disjunction tests.


<===>
<==============================================================================>
<===> color/equal/input.scss
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like color-to-color equality warrants substantially more tests. If you want to delay it for now, that's fine, but if so add a README with a TODO that links to #1418.

@@ -0,0 +1,106 @@
<===> boolean/true/input.scss
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than have separate inequality tests that wholly duplicate the equality tests, it's probably better to write a helper mixin that verifies that two values are equal according to both == and !=. I think it's reasonable to delay this until #1418 as well, in which case maybe just leave out inequality specs for now.

@@ -0,0 +1,47 @@
<===> keyword/input.scss
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these specs should be substantially expanded:

  • We should verify that these values are actually parsed as colors, not just that they're emitted as written. Maybe pass them to adjust-color() with no other args, which will both verify that its argument is a color and return a color value without an associated string representation?

  • We need to all color names Sass supports, and that those names are case-insensitive.

  • We need to verify that hex digits are parsed correctly into red/green/blue/alpha channels for all hex representations.

@nex3 nex3 closed this Sep 13, 2022
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.

2 participants