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

Add keyframe-block-no-duplicate-selectors #6024

Merged
merged 7 commits into from Apr 21, 2022

Conversation

mattxwang
Copy link
Member

Which issue, if any, is this issue related to?

Closes #5991.

Is there anything in the PR that needs further explanation?

Still newish to the overall codebase! A couple of questions I had (and of course, general feedback is much appreciated):

  1. Should I explicitly be handling case-insensitivity? Alternatively, should I document that the rule is case-sensitive (as of now)
  2. How comprehensive are the test cases and documentation? I'm not sure if I'm missing any common cases where this would be a problem.
  3. I've added vendored prefixes to the regex for finding keyframes as well as in the test cases - is this good practice?

@mattxwang
Copy link
Member Author

For code coverage - is there a specific way I should be testing the rule with invalid options? That's the only uncovered line I have.

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.

@mattxwang Thanks for creating the pull request. 👍🏼

Additionally, it should be better to support end positions for warnings.
See also #5694 (comment)

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.

@mattxwang Thank you! Looking good so far.

As @ybiquitous said, we'll want to add column and line numbers (including end ones) to the reject tests.

I've requested a few changes myself.

@jeddy3
Copy link
Member

jeddy3 commented Apr 19, 2022

Should I explicitly be handling case-insensitivity?

Yes. Let's add @keyframes foo { from {} FROM {} } as a reject test.

How comprehensive are the test cases and documentation?

They're comprehensive enough, thank you.

I've added vendored prefixes to the regex for finding keyframes as well as in the test cases - is this good practice?

Yes, LGTM.

For code coverage - is there a specific way I should be testing the rule with invalid options?

You can ignore the coverage warning.

@mattxwang
Copy link
Member Author

Thank you @jeddy3 and @ybiquitous for the in-depth feedback - end positions and non-standard syntax slipped my mind! I've pushed changes to address the feedback; let me know if you have any other thoughts.

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.

@mattxwang Nice! LGTM 👍🏼

lib/rules/keyframe-block-no-duplicate-selectors/index.js Outdated Show resolved Hide resolved
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
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.

@mattxwang Thanks for making the changes. LGTM.

@jeddy3 jeddy3 merged commit 08df98c into main Apr 21, 2022
@jeddy3 jeddy3 deleted the add-keyframe-block-no-duplicate-selectors branch April 21, 2022 12:47
@jeddy3
Copy link
Member

jeddy3 commented Apr 21, 2022

  • Added: keyframe-block-no-duplicate-selectors rule (#6024).

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.

Add keyframe-block-no-duplicate-selectors
3 participants