-
-
Notifications
You must be signed in to change notification settings - Fork 945
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
Fix false positives for variables and color functions in color-function-notation #5793
Conversation
Question here for maintainers. The rules that are failing now from the scss syntax section: stylelint/lib/rules/color-function-notation/__tests__/index.js Lines 407 to 413 in d630d96
stylelint/lib/rules/color-function-notation/__tests__/index.js Lines 441 to 446 in d630d96
Are these rules valid tests cases here? Should this be something that is handled by stylelint-scss? It seems like part of the problem is that the contents of the variable is what needs to be checked first and that's not a responsibility of stylelint. |
Thanks for the pull request!
Nope. Those should be accepted now, rather than rejected.
Yes, exactly. |
Looking closer at theses tests, is the scss section necessary at all or should I merge those test cases with the main syntax rules? |
They should be in a separate |
Got it, thanks. This one is ready for review now. |
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.
@lachieh Thank you for creating this pull request!
It would seem better to me if you could add some test cases to isStandardSyntaxColorFunction.test.js
.
@ybiquitous Thanks, added some test cases. Let me know if it needs any more. |
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.
@lachieh Thank you! LGTM 👍🏼
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.
LGTM, thank you!
@jeddy3 Will you update the changelog? |
Whoops, good catch. Thanks @ybiquitous ! Added now:
I'll be releasing shortly. |
Fixes #5671