-
-
Notifications
You must be signed in to change notification settings - Fork 927
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 selector-max-specificity
end positions
#7685
Fix selector-max-specificity
end positions
#7685
Conversation
🦋 Changeset detectedLatest commit: 56269fe The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
I will take a look at the coverage issue shortly. |
…c6d1a' of https://github.com/stylelint/stylelint into fix-selector-max-specificity--adventurous-pelican-112c3c6d1a
@@ -65,6 +65,9 @@ testRule({ | |||
{ | |||
code: ':nth-child(2n + 1) {}', | |||
}, | |||
{ | |||
code: '[value] {}', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
non-blocking
It's kind of contradictory.
i.e. that's the attribute name not its value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM 👍🏼
// Check if the selector specificity exceeds the allowed maximum | ||
if (compare(maxChildSpecificity(selectorTree), maxSpecificity) > 0) { | ||
if (compare(nodeSpecificity(resolvedSelector), maxSpecificity) > 0) { | ||
const index = selector.first?.sourceIndex ?? 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[note] The coverage diff is caused by this line, but I think it's ignorable.
If improving the coverage, we might change it like this, but I'm unsure it's really safe...
const index = selector.first?.sourceIndex ?? 0; | |
const index = selector.first.sourceIndex; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think it is safe in practice, but I'd rather keep it anyway.
If the AST is mutated without setting sourceIndex
or if something clears out a selector it would throw errors.
But maybe we can add a helper so that we can document this better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with keeping it as-is, and I agree with adding a helper. 👍🏼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should actually fix this one : postcss/postcss-selector-parser#287
Then we change const index = selector.first?.sourceIndex ?? 0
to const index = selector.sourceIndex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice👍
See : #6234
No, it's self-explanatory.