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

No color literals raises warning on variable in map #538

Closed
brettlangdon opened this issue Feb 17, 2016 · 6 comments · Fixed by #647
Closed

No color literals raises warning on variable in map #538

brettlangdon opened this issue Feb 17, 2016 · 6 comments · Fixed by #647

Comments

@brettlangdon
Copy link
Contributor

I am seeing an issue where a warning is raised when a color variable is used as a map value. Specifically when the variable shares the same name as a valid CSS color.

To reproduce:

$black: #000;
$colors: (
  font-color: $black,
);
$ sass-lint --verbose ./test.scss
./test.scss
  3:16  warning  Color literals such as 'black' should not be used as map identifiers  no-color-literals

✖ 1 problem (0 errors, 1 warning)

I am running sass-lint 1.5.0.

@brettlangdon
Copy link
Contributor Author

I am working on a pull request for this issue.

The fix I was able to get working was to ensure that the map value's parent was not a variable.

@DanPurdy
Copy link
Member

The reason we decided against was because it actually used to raise the same warnings in scss-lint and we were looking for parity. This fix may have to wait until our 1.6 update is out as we've changed a few of the rules a fair bit. Thankyou though for taking the time, I'm sure we're far along enough now to diverge from scss-lint

@brettlangdon
Copy link
Contributor Author

More than happy to wait for 1.6, I have changed my code to use my fork/branch for now anyways, so I can live without this being merged for a little bit.

My only thoughts on the warning are that they are based on the name of the variable, but there isn't a corresponding rule for that variable name not being allowed, the warning would fit better if there was also a rule that disallowed common CSS color names as variables.

@DanPurdy
Copy link
Member

Yep, I think that makes perfect sense. The whole ethos with sass-lint has been for small focused rules so that would suit perfectly.

Thanks again

@webgefrickel
Copy link

First things first: thanks so much for the long awaited 1.6.0 — almost all issues I had are gone now, well except for this one, I can reproduce this behaviour with 1.6.0. Any ideas on this one?

@DanPurdy
Copy link
Member

@webgefrickel thanks for the kind words, we're glad 1.6 is sorting a lot of problems out, it's a massive relief for us to get 1.6.0 out with the work we've had to get into it including getting some big issues sorted out in the AST.

As for this issue there's a PR currently open that now 1.6.0 is out we'll be looking to get merged in soon!
#539

There should be a patch release very very shortly too to fix a few of the extra issues we've come across and more regular updates should now be around as we've got the structural change and AST update out of the way!

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 a pull request may close this issue.

3 participants