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 lint for rtl:ignore comment directives on nested style rules #893

Open
Goodwine opened this issue Dec 6, 2023 · 3 comments
Open

Add lint for rtl:ignore comment directives on nested style rules #893

Goodwine opened this issue Dec 6, 2023 · 3 comments

Comments

@Goodwine
Copy link

Goodwine commented Dec 6, 2023

Input

.foo {
  /* rtl:ignore */
  .bar {
    a: b;
  }
}

Output (playground)

.foo {
  /* rtl:ignore */      <------ This was not moved out.
}
.foo .bar {             <------ BUG: This is not ignored by RTLCSS as intended.
  a: b;
}

Context

/* rtl:ignore */ is an RTLCSS directive which makes RTLCSS skip the subsequent CSS node from being RTL-yfied. For example: /* rtl:ignore */ .foo { float:left; margin-right: 10px; } causes both declarations to remain unchanged by RTLCSS.

However, Sass loud comments (/* */) within a rule declaration aren't attached to anything. They don't "belong" to the subsequent node. And therefore, when Sass compiles to CSS, it "splits" the directive away from the nested rule block which the author intended as shown in the example above.

And to add a cherry on top, this could lead to a bug if there exists a declaration after the nested rule, like in this example:

.foo {
  /* rtl:ignore */
  .bar {
    a: b;
  }

  float: left;
}

Which produces:

.foo {
  /* rtl:ignore */
  float: left;      <----- BUG 2: This is now ignored by RTLCSS, but that wasn't intended.
}
.foo .bar {
  a: b;
}
@Goodwine
Copy link
Author

Goodwine commented Dec 6, 2023

BTW I'm looking to work on this this week unless :)

@kristerkari
Copy link
Collaborator

kristerkari commented Dec 8, 2023

@Goodwine Thanks for the detailed description!

Looking at what you wrote, I started wondering if the problem is maybe a bit too specific to RTLCSS to be added as a rule to this library. I understand that your problem is related to Sass, but I bet that most of the users of stylelint-scss are not using RTLCSS.

Could this idea instead be turned into a separate stylelint plugin that would be published in npm? Stylelint supports the postcss-scss customSyntax, so it's really easy to make a rule support SCSS.

@Goodwine
Copy link
Author

Since this affects any loud comment (not just RTLCSS), would it make sense to still implement this lint check as part of this repo if it was made more generic?

For example:

  • check: Any loud comment that appears immediately before a nested style-rule or at-rule
  • finding text: "A loud comment (/**/) over a nested rule does not follow the rule when Sass compiles to CSS. Use a quiet comment (//) or unnest the rule." (or better wording)
  • auto-fix: transforms to quiet comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants