Navigation Menu

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

Fix false positives for variables and color functions in color-function-notation #5793

Merged
merged 5 commits into from Dec 21, 2021

Conversation

lachieh
Copy link
Contributor

@lachieh lachieh commented Dec 20, 2021

Fixes #5671

@lachieh lachieh changed the title add recursive check and add $ check Fix false positives for variables and color functions in color-function-notation Dec 20, 2021
@lachieh
Copy link
Contributor Author

lachieh commented Dec 20, 2021

Question here for maintainers. The rules that are failing now from the scss syntax section:

{
code: 'a { color: rgba($a, $b, $c, $d) }',
fixed: 'a { color: rgb($a $b $c / $d) }',
message: messages.expected('modern'),
line: 1,
column: 12,
},

{
code: 'a { color: rgb($a $b $c / $d) }',
message: messages.expected('legacy'),
line: 1,
column: 12,
},

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.

@jeddy3
Copy link
Member

jeddy3 commented Dec 20, 2021

Thanks for the pull request!

Are these rules valid tests cases here?

Nope. Those should be accepted now, rather than rejected.

Should this be something that is handled by stylelint-scss?

Yes, exactly.

@lachieh
Copy link
Contributor Author

lachieh commented Dec 20, 2021

Looking closer at theses tests, is the scss section necessary at all or should I merge those test cases with the main syntax rules?

@lachieh lachieh marked this pull request as ready for review December 20, 2021 15:14
@jeddy3
Copy link
Member

jeddy3 commented Dec 20, 2021

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 testRule (that uses the postcss-scss syntax - even though the default parser can parse $) so that we signal that we're testing non-standard constructs (which, in this case, are dollar variables - both defined and used). It helps us maintain the line between CSS and non-standard stuff.

@lachieh
Copy link
Contributor Author

lachieh commented Dec 20, 2021

Got it, thanks. This one is ready for review now.

Copy link
Member

@ybiquitous ybiquitous left a 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.

@lachieh
Copy link
Contributor Author

lachieh commented Dec 20, 2021

@ybiquitous Thanks, added some test cases. Let me know if it needs any more.

Copy link
Member

@ybiquitous ybiquitous left a 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 👍🏼

Copy link
Member

@jeddy3 jeddy3 left a 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 jeddy3 merged commit 217e685 into stylelint:main Dec 21, 2021
@ybiquitous
Copy link
Member

@jeddy3 Will you update the changelog?

@jeddy3
Copy link
Member

jeddy3 commented Dec 21, 2021

Whoops, good catch. Thanks @ybiquitous !

Added now:

  • Fixed: color-function-notation false positives for variables and color functions (#5793)

I'll be releasing shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Fix false positives for variables and color functions in color-function-notation
3 participants