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-selector-notation #6164

Merged
merged 7 commits into from Jun 23, 2022
Merged

Add keyframe-selector-notation #6164

merged 7 commits into from Jun 23, 2022

Conversation

mattxwang
Copy link
Member

@mattxwang mattxwang commented Jun 20, 2022

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

Closes #4065.

Is there anything in the PR that needs further explanation?

Not sure if I'm implementing the detect & autofix features properly; I've made some comments below. My approach is inspired by alpha-value-notation.

@mattxwang mattxwang changed the title Add keyframe-selector-notation Add keyframe-selector-notation Jun 20, 2022
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 Thank you for your excellent work!

I think this pull request looks almost good though I've left some trivial comments. 👍🏼

docs/user-guide/rules/list.md Outdated Show resolved Hide resolved
lib/rules/keyframe-selector-notation/index.js Outdated Show resolved Hide resolved
lib/rules/keyframe-selector-notation/index.js Outdated Show resolved Hide resolved
lib/rules/keyframe-selector-notation/README.md Outdated Show resolved Hide resolved
lib/rules/keyframe-selector-notation/index.js Show resolved Hide resolved
Copy link
Member Author

@mattxwang mattxwang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick comments! Let me see if I can rework the code given your suggestion.

mattxwang and others added 2 commits June 20, 2022 23:30
Typo fixes and type hints

Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
@mattxwang
Copy link
Member Author

Hm, the failed actions aren't me - it's a 503 on NPM's registry with npm install --global npm@latest.

Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
@ybiquitous
Copy link
Member

ybiquitous commented Jun 21, 2022

Hm, the failed actions aren't me - it's a 503 on NPM's registry with npm install --global npm@latest.

It seems to be due to the Cloudflare outage:
https://www.cloudflarestatus.com/

EDIT: https://status.npmjs.org/incidents/8cgldky16yk4

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.

Looking great! Thank you.

Let's add tests for a couple of edge-cases that are valid syntax:

  • duplicate keyframe selectors, e.g. @keyframes foo { from {} from {} to {} }
  • mixed notations, e.g. @keyframes foo { from {} 50% {} to {} }
  • keyframe selectors lists, e.g. @keyframes foo { from, to {} }

Let's add accept and reject tests for each of these.

docs/user-guide/rules/list.md Outdated Show resolved Hide resolved
lib/rules/keyframe-selector-notation/README.md Outdated Show resolved Hide resolved
mattxwang and others added 2 commits June 21, 2022 22:07
Minor naming fix, remove unnecessary description

Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
@mattxwang
Copy link
Member Author

Thanks @jeddy3 for the suggestion! Upon adding the test cases, I realized I didn't properly handle the selector list use-case. I've written a bit of a hacky solution that requires the selector parser, walks the tags, and then regenerates the selector; while it works, it doesn't feel like the most elegant solution. Do you have any suggestions (or places to look) for a better way to do this?

@jeddy3
Copy link
Member

jeddy3 commented Jun 22, 2022

@mattxwang Thanks for making the changes. I think it's a valid use of the selector parser. @ybiquitous may have some thoughts on the code structure as he's done lots of refactoring lately to make our code consistent across rules.

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.

How about our transformSelector() utility?

module.exports = function transformSelector(result, node, callback) {

lib/rules/keyframe-selector-notation/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.

Thanks for making all the changes.

LGTM!

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.

Thank you! LGTM 👍🏼

@jeddy3 jeddy3 merged commit cd8710c into main Jun 23, 2022
@jeddy3 jeddy3 deleted the add-keyframe-selector-notation branch June 23, 2022 16:26
@jeddy3
Copy link
Member

jeddy3 commented Jun 23, 2022

  • Added: keyframe-selector-notation rule (#6164).

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-selector-notation
3 participants