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

Add ignore: ["inside-block"] to selector-disallowed-list #6294

Closed
yairEO opened this issue Aug 21, 2022 · 8 comments · Fixed by #6334
Closed

Add ignore: ["inside-block"] to selector-disallowed-list #6294

yairEO opened this issue Aug 21, 2022 · 8 comments · Fixed by #6334
Labels
good first issue is good for newcomers status: ready to implement is ready to be worked on by someone type: new option a new option for an existing rule

Comments

@yairEO
Copy link

yairEO commented Aug 21, 2022

What is the problem you're trying to solve?

I also want to disallow top-level selectors which are starting with & because that's invalid CSS code

&.foo { }

(common to SASS precompiler language but is not restricted to it)

What solution would you like to see?

either top-level flexible regex rule or an explicit rule for anything starting specifically with &

@yairEO yairEO changed the title feature - disallow top-level selectors starting with & feature request - disallow top-level selectors starting with & Aug 21, 2022
@ybiquitous
Copy link
Member

I've closed this issue because of not using the issue template. Please provide more details like background, reason, use cases, etc.

@ybiquitous ybiquitous closed this as not planned Won't fix, can't repro, duplicate, stale Sep 2, 2022
@ybiquitous ybiquitous reopened this Sep 2, 2022
@ybiquitous ybiquitous added the status: needs discussion triage needs further discussion label Sep 3, 2022
@ybiquitous
Copy link
Member

@yairEO Thanks for using the template. I understand your motivation.

Your request seems to be satisfied by validators like the stylelint-csstree-validator plugin, the direct use of CSSTree, or Sass compiler, but is it correct?

CSSTree:

Screen Shot 2022-09-03 at 18 34 25

Sass:

Screen Shot 2022-09-03 at 18 35 26

See also our document about validators.

@jeddy3 jeddy3 changed the title feature request - disallow top-level selectors starting with & Add ignore: ["inside-block"] to selector-disallowed-list Sep 3, 2022
@jeddy3
Copy link
Member

jeddy3 commented Sep 3, 2022

This is an interesting one.

According to the latest draft spec &.foo {} is a valid CSS:

When used in the selector of a nested style rule, the nesting selector represents the elements matched by the parent rule. When used in any other context, it represents nothing. (That is, it’s valid, but matches no elements.)

I think the CSSTree validator is currently flagging it as doesn't parse nesting outright yet, e.g.:

Screenshot 2022-09-03 at 15 14 23

Stylelint is a good fit for CSS that is valid, but probably wasn't what the author intended to write. It'd be inline with the other *-unmatchable rules we're thinking of adding (e.g. #5907)). However, we probably don't want to add any new nesting rules just yet as the nesting specification is influx (with the syntax still to be settled on).

I can see us adding a whole suite of rules for nesting when the specification does stabilise, though. For example, there is plenty of other valid nesting CSS that a person would probably want to limit:

a {
  && {}
}

Or enforce a particular notation, e.g. these two are equivalent:

a {
  @media (orientation: landscape) { color: red; }
}

a {
  @media (orientation: landscape) { & { color: red; } }
}

I suggest that anyone who is interested in these create plugins that we can port over to built-in rules once the specification stabilises.

In the meantime, we can add an ignore: ["inside-block"] option to the selector-disallowed-list rule (example of prior art). A user can then craft a regex to disallow the nesting selector (&) anywhere in a selector unless the selector is inside of a block.

It's not a perfect solution as the option will apply to all the items in the array, but it'll probably address @yairEO immediate use case.

@yairEO
Copy link
Author

yairEO commented Sep 3, 2022

The spec will be implemented in new browser, while many of today's projects need to support older browsers, of probably 2+ years old, and in that case, even if the spec says something, it's irrelevant for those mentioned projects since that CSS results a bug, which a rule should prevent.

@jeddy3 jeddy3 added status: ready to implement is ready to be worked on by someone type: new option a new option for an existing rule good first issue is good for newcomers and removed status: needs discussion triage needs further discussion labels Sep 4, 2022
@jeddy3
Copy link
Member

jeddy3 commented Sep 4, 2022

@yairEO I've labelled the issue as ready to implement. Please consider contributing if you have time.

There are steps on how to add a new option in the Developer guide.

@yairEO yairEO closed this as completed Sep 4, 2022
@yairEO yairEO reopened this Sep 4, 2022
@mattmanuel90
Copy link
Contributor

mattmanuel90 commented Sep 5, 2022

@yairEO

Glad I found this. I was about to create this issue. Happy for you to raise the PR. Otherwise, I can do it if you don't mind as we'd like this fix for a different use case.

Ours is to disallow root level classes used in our UI lib, to be used at our application codebase to avoid global styling override issues. We just want to allow overriding if done inside a block of a page level class.

The solution just involves disallowing selectors with a ruleNode.parent.type !== 'root' and it works pretty well if given the right regexp.

@jeddy3
Copy link
Member

jeddy3 commented Sep 5, 2022

The solution just involves disallowing selectors with a ruleNode.parent.type !== 'root' and it works pretty well if given the right regexp.

That's correct. It'd just be the case of returning early if the ignore: ["inside-block"] option is set and the ruleNode.parent.type !== 'root' condition is true.

Glad I found this. I was about to create this issue. Happy for you to raise the PR. Otherwise, I can do it if you don't mind as we'd like this fix for a different use case.

@mattmanuel90 as @yairEO hasn't indicated they're working on it, you're welcome to create a pull request yourself.

@yairEO
Copy link
Author

yairEO commented Sep 5, 2022

I'm a single person, not "they".. I don't have the time capacity as I already am the author of dozens of open-source projects and have a fulltime job and also need to find the time watching funny cats YouTubes while eating watermelons and thus more busy than Elon MusK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue is good for newcomers status: ready to implement is ready to be worked on by someone type: new option a new option for an existing rule
Development

Successfully merging a pull request may close this issue.

4 participants