Fix false positives for x unit within vendor-prefixed image-set in unit-no-unknown #4654
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lib/rules/unit-no-unknown/index.js
Outdated
| @@ -116,8 +117,10 @@ function rule(actual, options) { | |||
| return; | |||
| } | |||
|
|
|||
| if (/^image-set/i.test(value)) { | |||
| const imageSet = parsedValue.nodes.find((node) => node.value === 'image-set'); | |||
| if (/image-set/i.test(value)) { | |||
There was a problem hiding this comment.
I think better /^(?:-webkit-)?image-set$/i, because in theory we can have image-set-something() function (yes, it is not exists in CSS), but strict check always better
There was a problem hiding this comment.
Adding $ at the end does not work here because of what we're matching on. I was able to use [\s(] to delimit the end of the function name.
jeddy3
requested changes
Mar 20, 2020
There was a problem hiding this comment.
Thanks for the pull request.
Yes, let's go with tweaking the RegEx.
I think we'll be fine with /^(?:-webkit-)?image-set$/i as only the -webkit prefix is currently used.
jeddy3
changed the title
ybiquitous
approved these changes
Apr 2, 2020
| @@ -116,8 +117,10 @@ function rule(actual, options) { | |||
| return; | |||
| } | |||
|
|
|||
| if (/^image-set/i.test(value)) { | |||
| const imageSet = parsedValue.nodes.find((node) => node.value === 'image-set'); | |||
| if (/^(?:-webkit-)?image-set[\s(]/i.test(value)) { | |||
There was a problem hiding this comment.
[IMO] If using \b (word boundary), the intent of the code may be clear.
For example:
Suggested change
| if (/^(?:-webkit-)?image-set[\s(]/i.test(value)) { | |
| if (/^(?:-webkit-)?image-set\b/i.test(value)) { |
|
Changelog entry:
|
This was referenced Apr 5, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #4653
No, it's self-explanatory.
However, I'm not super familiar with the ASTs involved, and I'm super open to suggestions if this is not an optimal fix.