feat: add no-important rule#124
Conversation
nzakas
left a comment
nzakas
left a comment
There was a problem hiding this comment.
This is looking really good. Just a few suggestions to clean things up.
lumirlumir
left a comment
lumirlumir
left a comment
There was a problem hiding this comment.
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.
| - 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.
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.
Yes, I think that would be fine too.
| }, | ||
|
|
||
| messages: { | ||
| unexpectedImportant: "Unexpected !important flag found.", |
There was a problem hiding this comment.
| unexpectedImportant: "Unexpected !important flag found.", | |
| unexpectedImportant: "Unexpected `!important` flag found.", |
Same as above.
There was a problem hiding this comment.
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.
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.
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.
nzakas
left a comment
nzakas
left a comment
There was a problem hiding this comment.
LGTM. Would like @lumirlumir to verify before merging.
4 participants
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?