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

New Rule: no-important #20

Open
1 task done
aryan02420 opened this issue Nov 27, 2024 · 7 comments
Open
1 task done

New Rule: no-important #20

aryan02420 opened this issue Nov 27, 2024 · 7 comments
Labels

Comments

@aryan02420
Copy link

Rule details

Flag the use of !important

What type of rule is this?

Warns about a potential problem

Example code

.really-red {
    color: red !important;
}

Participation

  • I am willing to submit a pull request to implement this rule.

Additional comments

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

@jeddy3
Copy link

jeddy3 commented Nov 27, 2024

As with #19, a no-restricted-syntax rule could cover this use case:

{
    "rules": {
        "no-restricted-syntax": [
            "error",
            {
                "selector": "Declaration[important=true]",
                "message": "!important annotations are not allowed."
            },
        ]
    }
}

When using cascade layers, the !important annotation is useful:

Once we start using cascade layers, we will need to be much more cautious and intentional about how we use !important. It’s no longer a quick way to jump to the top of the priorities — but an integrated part of our cascade layering; a way for lower layers to insist that some of their styles are essential.

Fewer people will likely want to restrict this syntax as they adopt cascade layers.

@nzakas
Copy link
Member

nzakas commented Nov 27, 2024

Yeah, I think we want to hold off on adding rules that simply disallow syntax because it can easily be covered by no-restricted-syntax, but leaving open for now to see if others feel differently.

@yannbertrand
Copy link

I was excited about that one but reading the comments makes me doubt now 😀. I believe @jeddy3's view on these is super interesting, I personally have 0 background on CSS linting so following your advice seems like the best idea to me!

@nzakas
Copy link
Member

nzakas commented Nov 27, 2024

@jeddy3 is this true for keyframe !important as well? I noticed Stylelint has two different rules for !important:

https://stylelint.io/user-guide/rules/keyframe-declaration-no-important/
https://stylelint.io/user-guide/rules/declaration-no-important/

@jeddy3
Copy link

jeddy3 commented Nov 28, 2024

The declaration-no-important rule is for restricting valid syntax, whereas the keyframe-declaration-no-important one flags invalid !important annotations within keyframe blocks:

The <declaration-list> inside of <keyframe-block> accepts any CSS property... None of the properties interact with the cascade (so using !important on them is invalid and will cause the property to be ignored).

We added the declaration-no-important rule to Stylelint at a time when using !important was universally considered bad. This no longer appears to be the case, especially when using cascade layers.

How about?:

  1. Repurposing feat: add no-important rule #23 to disallow only invalid !important annotations and turning it on in your recommended config.
  2. Adding a no-restricted-syntax rule, using valid !important annotations as an example, and not turning it on in your recommended config.

You could either limit the repurposed #23 to keyframe blocks (as no-invalid-important) or extend it to flag other invalid annotations (as no-invalid-annotations), e.g.:

a {
  color: red !foo;
}

(You may want to allow <dashed-ident> annotations, e.g. !--foo, as author-controlled syntax seems to be going that way.)

I think no-invalid-annotations, rather than no-invalid-important, would be in keeping with the scopes of the no-invalid-at-rules and no-invalid-properties rules, and would cover both Stylelint's:

@fasttime fasttime added this to Triage Nov 28, 2024
@github-project-automation github-project-automation bot moved this to Needs Triage in Triage Nov 28, 2024
@mdjermanovic
Copy link
Member

2. Adding a no-restricted-syntax rule, using valid !important annotations as an example, and not turning it on in your recommended config.

The core no-restricted-syntax rule can already be used with any language, including css.

@jeddy3
Copy link

jeddy3 commented Dec 1, 2024

That's great! Thank you for pointing that out.

In addition to restricting !important annotations with:

"no-restricted-syntax": [
    "error",
    {
        "selector": "Declaration[important=true]",
        "message": "!important annotations are not allowed."
    },
]

I was also able to replicate nearly all of Stylelint's *-(dis)allowed-list rules.

From simple ones like function-disallowed-list, e.g. disallowing the var() and rgb() functions:

{
  selector: "Function[name=/\\b(var|rgb)\\b/]",
  message: "var() and rgb() functions are not allowed",
}

To pair matching ones like declaration-property-unit-disallowed-list, e.g. disallow the s unit for the animation and animation-duration properties:

{
  selector: "Declaration[property=/^animation/] Dimension[unit=s]",
  message: "Use `ms` instead of `s` for animation durations"
},

Because ESQuery supports the :has() selector, I could also replicate (and improve on the) rules that would require inspecting the attributes of descendant nodes like media-feature-name-unit-allowed-list, e.g. enforcing rem units for width features:

{
  selector: "FeatureRange:has(Identifier[name=width]) Dimension[unit!=rem]",
  message: "Use `rem` units for the `width` feature"
},

Catching the use of px units in both ranged media and container queries:

@media screen and (10px < width <= 768px) {}

div {
  container-type: inline-size;
  div {
    @container (width > 30px) {}
  }
}

The core no-restricted-syntax is an incredibly useful rule!

Are there any other core ones that can be used with CSS?

@jeddy3 jeddy3 mentioned this issue Dec 1, 2024
1 task
@nzakas nzakas moved this from Needs Triage to Evaluating in Triage Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Evaluating
Development

No branches or pull requests

5 participants