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

feat: add no-important rule #23

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

yannbertrand
Copy link

Prerequisites checklist

What is the purpose of this pull request?

Add a rule to avoid !important annotation usage.

What changes did you make? (Give an overview)

  • Created the rule
  • Added it to the recommended rules
  • Wrote the docs

Related Issues

See #20

Is there anything you'd like reviewers to focus on?

Implementation was not complicated but I'm not sure on how much details the docs should include.

(It's my first PR on ESlint, lmk if anything is wrong or if the rule is useless)

Copy link

linux-foundation-easycla bot commented Nov 27, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Member

@nzakas nzakas 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 getting this started, it looks really good. Just a few things to clean up.

src/rules/no-important.js Outdated Show resolved Hide resolved
src/rules/no-important.js Outdated Show resolved Hide resolved
start: node.loc.start,
end: {
line: node.loc.start.line,
column:
Copy link
Member

Choose a reason for hiding this comment

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

This will just highlight the property name. I think it would be better to find the location of the !important and set that as the location of the error.

"a { color: red; }",
"a { color: red; background-color: blue; }",
"a { color: red; transition: none; }",
"body { --custom-property: red; }",
Copy link
Member

Choose a reason for hiding this comment

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

I'd also add a test with just the word "important":

Suggested change
"body { --custom-property: red; }",
"body { --custom-property: red; }",
"body { --custom-property: important; }",

tests/rules/no-important.test.js Outdated Show resolved Hide resolved
],
},
],
});
Copy link
Member

Choose a reason for hiding this comment

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

Can you also add a test that has multiple properties with !important to ensure we're catching them all?


## Background

Needing !important indicates there may be a larger underlying issue.
Copy link
Member

Choose a reason for hiding this comment

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

We need more of a description here. 😄 Please see other rule docs for examples.

a .link {
font-size: padding: 10px 20px 30px 40px !important;
}
```
Copy link
Member

Choose a reason for hiding this comment

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

@nzakas
Copy link
Member

nzakas commented Dec 2, 2024

Thanks for your work on this. There's some ongoing discussion about whether or not we'd like to have this rule on #20, so we may need to think things through a bit before proceeding.

@yannbertrand
Copy link
Author

yannbertrand commented Dec 2, 2024

Thanks for your work on this. There's some ongoing discussion about whether or not we'd like to have this rule on #20, so we may need to think things through a bit before proceeding.

Sure thing, no problem! Should we keep this PR opened? I'll wait for a decision to either get back to work on this one or replace it by another PR depending on the expected scope.

@nzakas
Copy link
Member

nzakas commented Dec 3, 2024

Let's keep this one open for now. I'll just mark it as a draft while we try to figure things out.

@nzakas nzakas marked this pull request as draft December 3, 2024 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Blocked
Development

Successfully merging this pull request may close these issues.

2 participants