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 interpolation in keyframes-name-pattern #6265
Conversation
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.
@a-falkonia Thanks for opening the pull request. It's looking good.
I've requested some minor changes.
@@ -38,6 +39,10 @@ const rule = (primary) => { | |||
return; | |||
} | |||
|
|||
if (!isStandardSyntaxKeyframesName(value)) { |
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.
Let's move this to before the first conditionally as we normally check that things are standard before any other operation.
|
||
describe('isStandardSyntaxKeyframesName', () => { | ||
it('standard name', () => { | ||
expect(isStandardSyntaxKeyframesName('@keyframes slidein {}')).toBeTruthy(); |
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.
expect(isStandardSyntaxKeyframesName('@keyframes slidein {}')).toBeTruthy(); | |
expect(isStandardSyntaxKeyframesName('slidein')).toBeTruthy(); |
As the util is checking the keyframes-name, and not the whole @keyframe
.
We'll need to update the other tests too.
*/ | ||
|
||
module.exports = function (keyframesName) { | ||
// SCSS or Less interpolation |
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.
// SCSS or Less interpolation |
We can lose the comment. (The hasInterpolation
util checks for more than just Less or Sass interpolation).
@jeddy3 Thank you for your review! |
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.
@a-falkonia Thanks for creating the PR!
I've left some refactoring suggestions, but the change looks almost good. 👍🏼
@ybiquitous Thank you! I've made the updates. |
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.
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.
@a-falkonia Thanks for making the changes. All most there.
I've requested one set of minor changes to the tests.
|
||
describe('isStandardSyntaxKeyframesName', () => { | ||
it('standard name', () => { | ||
expect(isStandardSyntaxKeyframesName('slidein {}')).toBe(true); |
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.
expect(isStandardSyntaxKeyframesName('slidein {}')).toBe(true); | |
expect(isStandardSyntaxKeyframesName('slidein')).toBe(true); |
We're only testing the name, not the block. We'll need to remove the block from the other tests too.
@jeddy3 So so sorry for missing this. All the blocks should be removed 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.
Thank you!
Changelog entry added:
|
Closes #5997
No, it's self-explanatory.