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-max-pseudo-class rule #3195

Merged
merged 13 commits into from Mar 23, 2018

Conversation

3 participants
@Bilie
Contributor

Bilie commented Mar 5, 2018

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

#2805

Is there anything in the PR that needs further explanation?

I had to use --no-verify flag when committing, as I was getting:
✖ npm run prettier --write found some errors. Please fix them and try committing again.
But no errors were listed and I did not know how to fix this.

@jeddy3

jeddy3 requested changes Mar 8, 2018 edited

@Bilie Many thanks for the PR!

I've requested some changes based on the guidelines.

The tests are failing because of a linting error. You'll likely address this when adapting to code to account for nested selectors, though. FYI, you can use npm run lint to run the linting locally.

You'll need to wire-up the rule by adding it to the list of rules and the example config.

But no errors were listed and I did not know how to fix this.

I've not come across this before, I'm afraid. You can run npm run prettier:fix before committing to make sure your files are correctly formatted.

Thanks again! I'm personally a great fan of these limiting rules and so it's great to see a contributor add more!

description: "multiple selectors: fewer than max pseudo classes"
},
{
code: "a:not(:first-child) { color: pink; }",

This comment has been minimized.

@jeddy3

jeddy3 Mar 8, 2018

Member

Do we think this should fail as these *-max-* rules are scoped to the entire selector in a selector list?

accept: [
{
code: "a:first-child { color: pink; }",

This comment has been minimized.

@jeddy3

jeddy3 Mar 8, 2018

Member

Can you remove all the declarations from the rule-sets, please? e.g.

a:first-child {}

The rule is only interested in the selector and so an empty block is better as it allows the tests to be more focused.

code: "a:not(:first-child) { color: pink; }",
description: "two selectors with one pseudo class each"
}
],

This comment has been minimized.

@jeddy3

jeddy3 Mar 8, 2018

Member

Can we an accept test for a nested selector, pleased e.g.

a {
  &:hover {}
}
{
code: "a:first-child { color: pink; }",
message: messages.expected("a:first-child", 0)
}

This comment has been minimized.

@jeddy3

jeddy3 Mar 8, 2018

Member

Can we please get a reject test for a nested selector? e.g.

a {
  &:hover {}
}
The following patterns are considered violations:
```css
a:first:focus { color: pink; }

This comment has been minimized.

@jeddy3

jeddy3 Mar 8, 2018

Member

Can we remove the declarations, please? And just have an empty block {}.

code: "a:first-child { color: pink; }",
message: messages.expected("a:first-child", 0)
}
]

This comment has been minimized.

@jeddy3

jeddy3 Mar 8, 2018

Member

Can we please get a reject test for a selector on the 2nd line of a selector list? e.g.

a,\na:first-child {}
@jeddy3

This comment has been minimized.

Member

jeddy3 commented Mar 15, 2018

@Bilie Friendly ping to see if you're still working on this PR?

@Bilie

This comment has been minimized.

Contributor

Bilie commented Mar 16, 2018

Hi @jeddy3 Thank you very much for all your feedback, it was really helpful, and for the ping :) I have updated the PR, I hope all issues you mentioned have been resolved!

@jeddy3

@Bilie Thanks for incorporating the feedback. It's looking good!

I've made some further, mostly minor, requests around the README and tests.

@@ -155,6 +155,7 @@ Here are all the rules within stylelint, grouped first [by category](../../VISIO
- [`selector-max-compound-selectors`](../../lib/rules/selector-max-compound-selectors/README.md): Limit the number of compound selectors in a selector.
- [`selector-max-empty-lines`](../../lib/rules/selector-max-empty-lines/README.md): Limit the number of adjacent empty lines within selectors.
- [`selector-max-id`](../../lib/rules/selector-max-id/README.md): Limit the number of id selectors in a selector.
- [`selector-max-pseudo-class`](../../lib/rules/selector-max-pseudo-class/README.md): Limit the number of pseudo classes in a selector.

This comment has been minimized.

@jeddy3

jeddy3 Mar 17, 2018

Member

Please hyphenate pseudo-classes.

@@ -0,0 +1,43 @@
# selector-max-pseudo-class
Limit the number of pseudo classes in a selector.

This comment has been minimized.

@jeddy3

jeddy3 Mar 17, 2018

Member

Please hyphenate pseudo-classes.

## Options
`int`: Maximum pseudo classes allowed.

This comment has been minimized.

@jeddy3

jeddy3 Mar 17, 2018

Member

Please hyphenate pseudo-classes.

```css
a:first:focus {}

This comment has been minimized.

@jeddy3

jeddy3 Mar 17, 2018

Member

Please remove empty line

```css
.foo .bar:first-child:hover {}

This comment has been minimized.

@jeddy3

jeddy3 Mar 17, 2018

Member

Please remove empty line

| |
1 2 -- this selector contains two pseudo classes */
```

This comment has been minimized.

@jeddy3

jeddy3 Mar 17, 2018

Member

We should add same description from the other selector-max-* rules here:

This rule resolves nested selectors before counting the number of pseudo-classes in a selector. Each selector in a [selector list](https://www.w3.org/TR/selectors4/#selector-list) is evaluated separately.

The content of the `:not()` pseudo-class is also evaluated separately. The rule processes the argument as if it were an independent selector, and the result does not count toward the total for the entire selector.
The following patterns are considered violations:
```css
a:first:focus {}

This comment has been minimized.

@jeddy3

jeddy3 Mar 17, 2018

Member

first-child

},
{
code: "a {}",
description: "selector with no pseudo class"

This comment has been minimized.

@jeddy3

jeddy3 Mar 17, 2018

Member

Please hyphenate pseudo-classes in all the descriptions.

{
code: ".foo { .bar { .baz {} } }",
description: "nested selectors with no pseudo classes"
}

This comment has been minimized.

@jeddy3

jeddy3 Mar 17, 2018

Member

Can we get two simple pseudo-element tests here please, for lines 43-50 of the rule code? e.g.

{
  code: "a:before {}",
  description: "single level 2 puesdo-element"
},
{
  code: "a::before {}",
  description: "single level 3 puesdo-element"
}

@jeddy3 jeddy3 changed the title from #2805 Add selector-max-pseudo-class rule to Add selector-max-pseudo-class rule Mar 17, 2018

@jeddy3

jeddy3 approved these changes Mar 17, 2018

@Bilie Thanks for the further amends. Good job!

LGTM.

@jeddy3 jeddy3 referenced this pull request Mar 22, 2018

Closed

Release 9.2 #3236

6 of 6 tasks complete
@hudochenkov

Good job, @Bilie!

@jeddy3 jeddy3 merged commit f23d5c8 into stylelint:master Mar 23, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.003%) to 95.522%
Details
@jeddy3

This comment has been minimized.

Member

jeddy3 commented Mar 23, 2018

  • Added: selector-max-pseudo-class rule (#3195).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment