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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃彈 Lint for $ in animation-name #30297

Merged
merged 14 commits into from Sep 19, 2020

Conversation

alanorozco
Copy link
Member

@alanorozco alanorozco commented Sep 18, 2020

Will error:

  const JSS = {
    foo: {
+ // The animation name in property animationName should start with $ (e.g. $foo).
+ // This scopes it to a keyframes rule present in this module.
      animationName: 'bar',
+ // ----------------^
    },
  };
  const JSS = {
    foo: {
+ // The animation name in property animation should start with $.
+ // This scopes it to a keyframes rule present in this module.
      animation: 'bar linear 0.5s',
+ // ------------^
    },
  };

@amp-owners-bot
Copy link

Hey @erwinmombay, @jridgewell! These files were changed:

build-system/eslint-rules/jss-animation-name.js

@samouri
Copy link
Member

samouri commented Sep 18, 2020

Two thoughts:

  1. Do we ever ever reference animation names from a different file, e.g. from ampshared?
  2. My personal preference for a rule like this is not to autofix and instead just have a good message. I personally have a workflow where I exclusively run lint with the --fix option, and this could just as easily hide an issue as fix it.

Copy link
Member

@samouri samouri left a comment

Choose a reason for hiding this comment

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

Had two small comments, overall lgtm

@alanorozco
Copy link
Member Author

  1. No, they're always set in their same CSS file, so it wouldn't break scoping.
  2. Fair enough, removed fix function.

build-system/eslint-rules/jss-animation-name.js Outdated Show resolved Hide resolved
build-system/eslint-rules/jss-animation-name.js Outdated Show resolved Hide resolved
alanorozco and others added 5 commits September 18, 2020 15:57
Co-authored-by: Justin Ridgewell <justin@ridgewell.name>
Co-authored-by: Justin Ridgewell <justin@ridgewell.name>
@alanorozco alanorozco merged commit 0cc9f10 into ampproject:master Sep 19, 2020
@alanorozco alanorozco deleted the jss-animation-name branch September 19, 2020 04:11
ed-bird pushed a commit to ed-bird/amphtml that referenced this pull request Dec 10, 2020
Will error:

```diff
  const JSS = {
    foo: {
+ // The animation name in property animationName should start with $ (e.g. $foo).
+ // This scopes it to a keyframes rule present in this module.
      animationName: 'bar',
+ // ----------------^
    },
  };
```

```diff
  const JSS = {
    foo: {
+ // The animation name in property animation should start with $.
+ // This scopes it to a keyframes rule present in this module.
      animation: 'bar linear 0.5s',
+ // ------------^
    },
  };
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants