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

Fix false positives for nested property declaration that contains tx or qx in unit-no-unknown #5434

Closed
Nittarab opened this issue Aug 6, 2021 · 4 comments · Fixed by #5500
Labels
good first issue is good for newcomers status: ready to implement is ready to be worked on by someone syntax: scss relates to SCSS and SCSS-like syntax type: bug a problem with a feature or rule

Comments

@Nittarab
Copy link

Nittarab commented Aug 6, 2021

Clearly describe the bug

There are false positives for unknown unite when you use tx or qx

Which rule, if any, is the bug related to?

unit-no-unknown

What code is needed to reproduce the bug?

e.g.

// Font Sizes
$font-sizes: (
    2x: $font-size-base * 2,
    2qx: $font-size-base * 2.25,
    2hx: $font-size-base * 2.5,
    tx: $font-size-base * 2.75,
    3x: $font-size-base * 3,
    3qx: $font-size-base * 3.25,
    3hx: $font-size-base * 3.5,
    3tx: $font-size-base * 3.75,
    4x: $font-size-base * 4,
    4qx: $font-size-base * 4.25,
    4hx: $font-size-base * 4.5,
    4tx: $font-size-base * 4.75,
    5x: $font-size-base * 5,
    5qx: $font-size-base * 5.25,
    5hx: $font-size-base * 5.5,
    5tx: $font-size-base * 5.75
)

What stylelint configuration is needed to reproduce the bug?

e.g.

{
  "rules": {
    "unit-no-unknown": "true"
  }
}

Which version of stylelint are you using?

e.g. 13.13.1

How are you running stylelint: CLI, PostCSS plugin, Node.js API?

yarn run stylelint --config .github/linters/.stylelintrc.json "app/**/*.scss"  

Does the bug relate to non-standard syntax (e.g. SCSS, Less etc.)?

Yes, it's related to SCSS nested properties declaration.

What did you expect to happen?

No warnings to be flagged.

What actually happened (e.g. what warnings or errors did you get)?

The following warnings were flagged:

 428:5  ✖  Unexpected unknown unit "qx"   unit-no-unknown
 432:5  ✖  Unexpected unknown unit "qx"   unit-no-unknown
 434:5  ✖  Unexpected unknown unit "tx"   unit-no-unknown
 436:5  ✖  Unexpected unknown unit "qx"   unit-no-unknown
 438:5  ✖  Unexpected unknown unit "tx"   unit-no-unknown
 440:2  ✖  Unexpected unknown unit "qx"   unit-no-unknown
 442:5  ✖  Unexpected unknown unit "tx"   unit-no-unknown
@jeddy3 jeddy3 changed the title Fix false positives for nested property declaration that contains tx or qx Fix false positives for nested property declaration that contains tx or qx in unit-no-unknown Aug 6, 2021
@jeddy3 jeddy3 added good first issue is good for newcomers status: ready to implement is ready to be worked on by someone syntax: scss relates to SCSS and SCSS-like syntax type: bug a problem with a feature or rule labels Aug 6, 2021
@jeddy3
Copy link
Member

jeddy3 commented Aug 6, 2021

@Nittarab Thanks for the report and for using the template.

I can reproduce using the demo with:

$foo: (
    bar: baz *,
    1unit: 1,
)

The bug seems specific to the * and , characters, and causes 1unit to be parsed as a value.

As PostCSS parses the whole construct as a declaration, I suggest we use the isStandardSyntaxDeclaration (around line 119) to ignore the construct. We may also need to update the conditionals in that util to ignore "declarations" whose value start with a ( and ends with a ).

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

There are steps on how to fix a bug in a rule in the Developer guide.

Please branch off v14 and not master.

@Nittarab
Copy link
Author

Thank you for the accurate answer, I don't have so much time during these days, but I'll try to fix it in the next few months

@mattxwang
Copy link
Member

Just poking around issues, was this closed by #5500? Seems like it, but want to make sure I'm not missing something.

@ybiquitous
Copy link
Member

@mattxwang Thank you for telling the issue us. We can close it!

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 syntax: scss relates to SCSS and SCSS-like syntax type: bug a problem with a feature or rule
Development

Successfully merging a pull request may close this issue.

4 participants