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

Added: `selector-max-attribute` rule. #2628

Merged
merged 1 commit into from Jun 20, 2017

Conversation

3 participants
@evilebottnawi
Member

evilebottnawi commented Jun 12, 2017

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

#2528

Is there anything in the PR that needs further explanation?

No, it's self explanatory.

Some Fixes in test selector-max-class:

  1. Remove properties, because it is unused for tests.
  2. Set config: [0] for less/sass syntax, because this construction should be ignored always, using 1 potential can to skip something, what we should ignore.
  3. .ab { .cd: { left: 0; top: 0; } } -> .ab { .cd { left: 0; top: 0; } } (seems just typo)
  4. .ab, .cd { &:hover > .ef.gh { left: 0; top: 0; } } -> .ab { &:hover > .ef.gh { left: 0; top: 0; } }, because for this case we have two errors, one for .ab, second for .cd. @davidtheclark @hudochenkov @jeddy3 can we throw error if we have more than one error on test block?
@jeddy3

jeddy3 requested changes Jun 12, 2017 edited

@evilebottnawi You're on fire! We'll have 7.12 out in not time at this rate :)

I've made some docs requests and two test requests around :not(). Otherwise, LGTM.

selector-max-class tests changes also LGTM.

@@ -162,6 +162,7 @@ Here are all the rules within stylelint, grouped by the [*thing*](http://apps.wo
- [`selector-combinator-space-before`](../../lib/rules/selector-combinator-space-before/README.md): Require a single space or disallow whitespace before the combinators of selectors.
- [`selector-descendant-combinator-no-non-space`](../../lib/rules/selector-descendant-combinator-no-non-space/README.md): Disallow non-space characters for descendant combinators of selectors.
- [`selector-id-pattern`](../../lib/rules/selector-id-pattern/README.md): Specify a pattern for id selectors.
- [`selector-max-attribute`](../../lib/rules/selector-max-attribute/README.md): Limit the number of attributes in a selector.

This comment has been minimized.

@jeddy3

jeddy3 Jun 12, 2017

Member

"Limit the number of attribute selectors in a selector."

@@ -0,0 +1,70 @@
# selector-max-attribute
Limit the number of attributes in a selector.

This comment has been minimized.

@jeddy3

jeddy3 Jun 12, 2017

Member

"Limit the number of attribute selectors in a selector."

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

This comment has been minimized.

@jeddy3

jeddy3 Jun 12, 2017

Member

This rule resolves nested selectors before counting the number of attribute selectors. Each selector in a selector list is evaluated separately.

## Options
`int`: Maximum attribute allowed.

This comment has been minimized.

@jeddy3

jeddy3 Jun 12, 2017

Member

Maximum attribute selectors allowed.

[type="text"][disabled]
```
```css

This comment has been minimized.

@jeddy3

jeddy3 Jun 12, 2017

Member

Let's put the comment on top. Long lines are hard to read.

/* each selector in a selector list is evaluated separately */
[type="text"][name="message"],
[type="number"][name="quality"] {}
[type="number"][name="quality"] {} /* each selector in a selector list is evaluated separately */
```
```css

This comment has been minimized.

@jeddy3

jeddy3 Jun 12, 2017

Member

Should this not be a selector list? e.g.

/* [data-attribute="value"]` is inside `:not()`, so it is evaluated separately */
[type="text"][name="message"]:not([data-attribute="value"]) {} 

This comment has been minimized.

@evilebottnawi

evilebottnawi Jun 12, 2017

Member

@jeddy3 not, example of list above, this example show attributes selectors and attributes selectors in not calculate separately

const ruleName = "selector-max-attribute"
const messages = ruleMessages(ruleName, {
expected: (selector, max) => `Expected "${selector}" to have no more than ${max} selector attribute`,

This comment has been minimized.

@jeddy3

jeddy3 Jun 12, 2017

Member
`Expected "${selector}" to have no more than ${max} attribute selector(s)`
code: "[type=\"text\"][disabled], \n[type=\"password\"][disabled] {}",
description: "multiple selectors: exactly max classes",
}, {
code: "[type=\"text\"][name=\"message\"] :not([type=\"number\"][name=\"quality\"]) {}",

This comment has been minimized.

@jeddy3

jeddy3 Jun 12, 2017

Member

This this be?:

[type=\"text\"][name=\"message\"]:not([type=\"number\"][name=\"quality\"]) {}

I'm not sure a compounded :not pseudo is valid CSS.

}, {
code: "[type=\"text\"][name=\"message\"][data-attribute=\"value\"] :not([disabled]) {}",
description: ":not(): greater than max classes, outside",
message: messages.expected("[type=\"text\"][name=\"message\"][data-attribute=\"value\"] :not([disabled])", 2),

This comment has been minimized.

@jeddy3

jeddy3 Jun 12, 2017

Member

Should be?:

[type=\"text\"][name=\"message\"][data-attribute=\"value\"]:not([disabled])

@evilebottnawi evilebottnawi force-pushed the selector-max-attribute branch from 0af4a53 to 34c3897 Jun 12, 2017

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Jun 12, 2017

/cc @jeddy3

[type="number"][name="quality"] {}
```
```css

This comment has been minimized.

@jeddy3

jeddy3 Jun 12, 2017

Member

I think we can condense the following two examples into just:

/* `[disabled]` is inside `:not()`, so it is evaluated separately */
[type="text"][name="message"]:not([disabled]) {}

As that shows 3 attribute selectors passing because the third attribute selector ([disabled]) is within a :not(). I think having a fourth attribute selector doesn't add anything to this example.

I think this makes the final example superfluous and can be deleted.

This comment has been minimized.

@evilebottnawi

evilebottnawi Jun 12, 2017

Member

@jeddy3 oh, yep, let's remove

This comment has been minimized.

@jeddy3

jeddy3 Jun 12, 2017

Member

And change the other example to:

/* `[disabled]` is inside `:not()`, so it is evaluated separately */
[type="text"][name="message"]:not([disabled]) {}

So there are 3, and not 4, attribute selectors.

@evilebottnawi evilebottnawi force-pushed the selector-max-attribute branch from 34c3897 to 02ad62b Jun 12, 2017

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Jun 12, 2017

@jeddy3 done

[type="number"][name="quality"] {}
```
```css

This comment has been minimized.

@jeddy3

jeddy3 Jun 12, 2017

Member

And change the other example to:

/* `[disabled]` is inside `:not()`, so it is evaluated separately */
[type="text"][name="message"]:not([disabled]) {}

So there are 3, and not 4, attribute selectors.

@evilebottnawi evilebottnawi force-pushed the selector-max-attribute branch from 02ad62b to 1afc90a Jun 12, 2017

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Jun 12, 2017

@jeddy3 seems done, sorry, for stupid error, I'm tired today, but I want to release 8.0.0 version faster

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jun 12, 2017

but I want to release 8.0.0 version faster

I think we all do :) It feels close now, though.

Thanks again for your effort with these rules!

@jeddy3

jeddy3 approved these changes Jun 12, 2017

Looks great. Thanks!

@hudochenkov

Good job!

Few minor fixes required.

For example, with `2`:
The following patterns are considered warnings:

This comment has been minimized.

@hudochenkov

hudochenkov Jun 15, 2017

Member

...violations.

This comment has been minimized.

@jeddy3

jeddy3 Jun 15, 2017

Member

Oops, didn't spot this. I had forgotten about this change :)

@evilebottnawi Please change to: "The following patterns are considered violations:"

input:not([type="text"][data-attribute="value"][disabled]) {}
```
The following patterns are *not* considered warnings:

This comment has been minimized.

@hudochenkov

hudochenkov Jun 15, 2017

Member

...violations.

const ruleName = "selector-max-attribute"
const messages = ruleMessages(ruleName, {
expected: (selector, max) => `Expected "${selector}" to have no more than ${max} attribute selector(s)`,

This comment has been minimized.

@hudochenkov

hudochenkov Jun 15, 2017

Member

Can we use this for nicer message?

`Expected "${selector}" to have no more than ${max} attribute ${max === 1 ? "selector" : "selectors"}`

This comment has been minimized.

@jeddy3

jeddy3 Jun 15, 2017

Member

I agree that this is nicer. However, I believe this style ("xxx(s)") is used for all max rules. Therefore I suggest we create a separate issue and make this change, at some point, across the board for the sake of consistency.

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Jun 15, 2017

can we throw error if we have more than one error on test block?

It's not supported in our jest-setup.js. I believe it's possible to do, if it's really necessary.

@evilebottnawi evilebottnawi force-pushed the selector-max-attribute branch from 1afc90a to a1c2ba1 Jun 20, 2017

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Jun 20, 2017

/cc @hudochenkov @jeddy3
Already some fixes in selector-max-class

@evilebottnawi evilebottnawi force-pushed the selector-max-attribute branch from a1c2ba1 to 8e3936d Jun 20, 2017

@hudochenkov

Looks great.

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Jun 20, 2017

@evilebottnawi I think squashing commits should be done after PR reviewed and accepted. This way it would be easier for reviewers to check what was changed between reviews ;)

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Jun 20, 2017

@hudochenkov sometimes there are lot of changes and commits and it is create noise for review PR (imho), seems this PR small and squashing it is not bad

@jeddy3 jeddy3 merged commit c9e8247 into master Jun 20, 2017

4 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.007%) to 95.638%
Details

@evilebottnawi evilebottnawi deleted the selector-max-attribute branch Jun 20, 2017

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jun 20, 2017

@evilebottnawi Thanks for this!

Changelog:

  • Added: selector-max-attribute rule (#2628).

Let's not squash commit during the review process. Like @hudochenkov I find it much easier to review changes when they are not squashed.

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