Skip to content

feat: add no-important rule #124

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

Merged
merged 3 commits into from
May 6, 2025
Merged

Conversation

thecalamiity
Copy link
Contributor

@thecalamiity thecalamiity commented May 3, 2025

Prerequisites checklist

What is the purpose of this pull request?

This pull request introduces a new rule, no-important, which disallows the use of the !important flag in declarations.

What changes did you make? (Give an overview)

  • Implemented the no-important rule to detect and warn when !important is used.
  • Added documentation for the rule
  • Added tests

Related Issues

Closes #20, Closes #23

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

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.

This is looking really good. Just a few suggestions to clean things up.

Copy link
Member

@lumirlumir lumirlumir left a comment

Choose a reason for hiding this comment

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

Overall, it looks good to me!

I've left a few comments for minor changes.

Comment on lines +9 to +12
- It breaks the natural cascade of CSS
- It makes debugging more difficult
- It can lead to specificity wars where developers keep adding more `!important` declarations to override each other
- It makes the code harder to maintain
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- It breaks the natural cascade of CSS
- It makes debugging more difficult
- It can lead to specificity wars where developers keep adding more `!important` declarations to override each other
- It makes the code harder to maintain
- It breaks the natural cascade of CSS.
- It makes debugging more difficult.
- It can lead to specificity wars where developers keep adding more `!important` declarations to override each other.
- It makes the code harder to maintain.

I believe adding a period would be appropriate, as it is a complete sentence and the items below also end with periods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I actually think both shouldn't end with periods since they are really phrases, not complete sentences?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think that would be fine too.

},

messages: {
unexpectedImportant: "Unexpected !important flag found.",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
unexpectedImportant: "Unexpected !important flag found.",
unexpectedImportant: "Unexpected `!important` flag found.",

Same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that the messages from other rules don't seem to wrap code snippets with backticks. So to maintain consistency I didn't implement this change. What do you think @nzakas?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, that makes sense for the internal CSS rules.

It might be better to open a separate PR to wrap code snippets in backticks, in order to follow the convention used in ESLint's internal rules.

Before proceeding, I'd like to hear thoughts from the ESLint team.

// @eslint/eslint-team

Copy link
Member

Choose a reason for hiding this comment

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

We don't wrap code inside of messages in backticks. These are rarely if ever rendered using Markdown. Sometimes we use quotes around syntax, and in this case, I think it's fine in the original form.

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.

LGTM. Would like @lumirlumir to verify before merging.

Copy link
Member

@lumirlumir lumirlumir left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

@nzakas nzakas merged commit af043db into eslint:main May 6, 2025
22 checks passed
@github-project-automation github-project-automation bot moved this from Needs Triage to Complete in Triage May 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

New Rule: no-important
4 participants