-
-
Notifications
You must be signed in to change notification settings - Fork 21
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
Conversation
There was a problem hiding this 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.
There was a problem hiding this 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.
- 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unexpectedImportant: "Unexpected !important flag found.", | |
unexpectedImportant: "Unexpected `!important` flag found.", |
Same as above.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Thanks!
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)
Related Issues
Closes #20, Closes #23
Is there anything you'd like reviewers to focus on?