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 selector-anb-no-unmatchable rule #6678

Merged
merged 13 commits into from
Feb 26, 2023

Conversation

mattxwang
Copy link
Member

@mattxwang mattxwang commented Feb 20, 2023

Which issue, if any, is this issue related to?

Closes #5907.

Is there anything in the PR that needs further explanation?

I've left a few comments in review.

In addition, I'm a bit confused about Grid-Structural Selectors. The spec itself doesn't have an example of :nth-col() or :nth-last-col(), the MDN docs' code example doesn't render for me on either Chrome or Firefox and has no browser compat data, and I haven't found much online on correct use. I must be missing something - some guidance would be helpful!

@changeset-bot
Copy link

changeset-bot bot commented Feb 20, 2023

🦋 Changeset detected

Latest commit: cc1e5cb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
stylelint Minor

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

Was unsure how to do grid structural selectors; can't test syntax in browser!
@mattxwang mattxwang force-pushed the selector-anb-no-unmatchable-csstree branch from fe4830a to 3ae61b7 Compare February 20, 2023 21:23
lib/rules/selector-anb-no-unmatchable/index.js Outdated Show resolved Hide resolved
Comment on lines 5 to 12
<!-- prettier-ignore -->
```css
a:nth-child(0n+0) {}
/*↑ ↑
* This unmatchable An+B selector */
```

[An+B selectors](https://drafts.csswg.org/css-syntax/#anb) are one-indexed. Selectors that always evaluate to `0` will not match any elements.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any suggestions on a better thing to highlight / extended description?

@mattxwang mattxwang marked this pull request as ready for review February 20, 2023 21:28
docs/user-guide/rules.md Outdated Show resolved Hide resolved
Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattxwang Thanks for the pull request, great work! 👏🏼

I've left minor suggestions, so please check them.

lib/rules/selector-anb-no-unmatchable/index.js Outdated Show resolved Hide resolved
lib/rules/selector-anb-no-unmatchable/index.js Outdated Show resolved Hide resolved
lib/rules/selector-anb-no-unmatchable/index.js Outdated Show resolved Hide resolved
@mattxwang
Copy link
Member Author

Thank you for the helpful feedback @ybiquitous! I need to get in the habit of adding messageArgs to new rules, and I agree that adding the : can make it easier for users to understand the message.

I think I've addressed all the comments, let me know what you think!

Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattxwang Thanks, LGTM! 👍🏼

I would suggest waiting for @jeddy3's review.

Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattxwang Thanks. It's looking great.

I've requested one significant change and some minor nits.

.changeset/cuddly-olives-learn.md Outdated Show resolved Hide resolved
docs/user-guide/rules.md Show resolved Hide resolved
lib/rules/selector-anb-no-unmatchable/README.md Outdated Show resolved Hide resolved
lib/rules/selector-anb-no-unmatchable/README.md Outdated Show resolved Hide resolved
lib/rules/selector-anb-no-unmatchable/README.md Outdated Show resolved Hide resolved
lib/rules/selector-anb-no-unmatchable/README.md Outdated Show resolved Hide resolved
lib/rules/selector-anb-no-unmatchable/README.md Outdated Show resolved Hide resolved
lib/rules/selector-anb-no-unmatchable/README.md Outdated Show resolved Hide resolved
lib/rules/selector-anb-no-unmatchable/README.md Outdated Show resolved Hide resolved
lib/rules/selector-anb-no-unmatchable/index.js Outdated Show resolved Hide resolved
mattxwang and others added 2 commits February 23, 2023 15:31
Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
@mattxwang
Copy link
Member Author

Thanks for the feedback @jeddy3; I think I've addressed all of the comments!

Two quick questions:

  1. should I add test cases for nonstandard selectors / situations where CSSTree will swallow the error? basically, the rule would accept/bail out here
    • as a note, this will increase the test coverage!
  2. should I also be doing grid-structural selectors? I was having some trouble finding good examples -- see PR description

@jeddy3
Copy link
Member

jeddy3 commented Feb 24, 2023

Thanks for making the changes:

should I add test cases for nonstandard selectors / situations where CSSTree will swallow the error?

Yeap, a single test case for that would be swell.

should I also be doing grid-structural selectors?

Let's leave them out for now.

@jeddy3 jeddy3 changed the title Addselector-anb-no-unmatchable Add selector-anb-no-unmatchable rule Feb 24, 2023
Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the changes. One minor request, then I think it LGTM.

lib/rules/selector-anb-no-unmatchable/__tests__/index.js Outdated Show resolved Hide resolved
Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for making the changes.

LGTM.

@mattxwang mattxwang merged commit ca6beab into main Feb 26, 2023
@mattxwang mattxwang deleted the selector-anb-no-unmatchable-csstree branch February 26, 2023 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add selector-anb-no-unmatchable
4 participants