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 negatives for css-in-js object notation in color-hex-case #5101
Conversation
As discussed in stylelint#4826, `style-search` has false positives when used with `css-in-js` object notation. This change updates the `color-hex-case` rule to use `postcss-value-parser` instead, which has support for `css-in-js` as a syntax. The unit tests now also include a suite for testing both `upper` and `lower` options for `color-hex-case` with the `css-in-js` syntax.
This is my first contribution, so let me know if there's something I missed with respect to formatting or code style. |
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.
@Dru89 Thanks! It's looking great so far, and so much cleaner now that the value parser is being used.
I've made a few requested changes.
Thanks for taking the time to look at all of this @jeddy3! I've gone through and made the changes you requested. For many of them (like the invalid five hex colors, e.g. Should we transform the original tests to also use more valid CSS? Or just leave them as they were before? |
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.
Fantastic, thanks for making the changes! It's almost there. Just one last tweak with the indents.
Should we transform the original tests to also use more valid CSS? Or just leave them as they were before?
Let's leave them as they are. We default to using valid CSS, but deviating in this case makes sense as it's testing something particular. Whereas in the testRule
for "css-in-js"
were testing that objects are read correctly, rather than the edge cases of the rule itself, and so valid CSS is appropriate here.
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.
@Dru89 Thanks. Looks great to me!
It's awesome to see the improvements with the refactoring to the value parser.
Thanks! I was gonna start in on some of the other Should I wait until this one gets the second approval and merged? Also, would you prefer one PR per rule? |
`style-search` returns false positives when used with `css-in-js` object notation. Continuing on the change made in stylelint#5101, this updates `color-hex-length` to switch to using `postcss-value-parser`. Unit tests have also been added to check the `css-in-js` syntax for this rule.
Fantastic!
No need to wait. Yes, one pull request per rule, please. |
lib/rules/color-hex-case/index.js
Outdated
|
||
return stringStart + replaceString + stringEnd; | ||
return decl; |
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.
Return is not used anywhere.
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.
@Dru89 It looks like this is the only change we need to make for a second approval.
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.
@Dru89 It looks like this is the only change we need to make for a second approval.
Sorry for the delay in getting this updated. Last week was a bit busier than I expected, but I've addressed that last piece of feedback. |
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.
👍
…#5106) * Use postcss-value-parser for color-hex-length `style-search` returns false positives when used with `css-in-js` object notation. Continuing on the change made in #5101, this updates `color-hex-length` to switch to using `postcss-value-parser`. Unit tests have also been added to check the `css-in-js` syntax for this rule. * Remove unnecessary return
As discussed in #4826,
style-search
has false positives when used withcss-in-js
object notation. This change updates thecolor-hex-case
rule to usepostcss-value-parser
instead, which has support forcss-in-js
as a syntax.The unit tests now also include a suite for testing both
upper
andlower
options forcolor-hex-case
with thecss-in-js
syntax.