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 ignore: ["pseudo-classes"] to max-nesting-depth #3724

Merged
merged 21 commits into from Nov 1, 2018

Conversation

3 participants
@inspiratweb
Contributor

inspiratweb commented Oct 16, 2018

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

Is there anything in the PR that needs further explanation?
Having max-nesting-depth = 1, it could be useful adding a rule to ìgnore` like

'max-nesting-depth': [1, {
  ignore: ["blockless-pseudoelements"]
}],

Allowing for example this css code

a {
  b { /* 1 */
    &:nest { /* 1 */
      &:nest-lvl2 { /* 1 */
        top: 0;
      }
    }
  }
}
@jeddy3

@inspiratweb Thanks for the PR!

I'm not sure the option is behaving as expected. I've asked for some clarifications in the comments below.

node.every(child => child.type !== "decl")) ||
(optionsMatches(options, "ignore", "blockless-pseudoclasses") &&
node.type === "rule" &&
node.some(child => child.selector && child.selector.startsWith("&:")))

This comment has been minimized.

@jeddy3

jeddy3 Oct 17, 2018

Member

I think you're missing the node.every(child => child.type !== "decl") conditional to enforce the "and do not themselves have declaration blocks." bit of the description.

This comment has been minimized.

@inspiratweb

inspiratweb Oct 18, 2018

Contributor

Sorry I c&p the at-rules block, I have to remove that description line.

code: "a { b { top: 0; }}"
},
{
code: "a { b { &:hover { top: 0; }}}"

This comment has been minimized.

@jeddy3

jeddy3 Oct 17, 2018

Member

I believe this should be rejected as there are two levels here:

  • the b blockless rule-set
  • the &:hover pseudo-class with a declaration

This comment has been minimized.

@inspiratweb

inspiratweb Oct 18, 2018

Contributor
a {
  b { // nesting level 1
    &:hover { // pseudo-class (parent depth level 1 taken)
      top: 0;
    }
  }
}
code: "a { b { &:hover { top: 0; }}}"
},
{
code: "a { b { &:nest { &:nest-lvl2 { top: 0; }}}}"

This comment has been minimized.

@jeddy3

jeddy3 Oct 17, 2018

Member

I believe this should be rejected as there are two levels here.

This comment has been minimized.

@inspiratweb

inspiratweb Oct 18, 2018

Contributor
a {
  b { // nesting level 1
    &:nest { // pseudo-class (parent depth level 1 taken)
      &:nest-lvl2 { // pseudo-class (parent depth level 1 taken)
        top: 0;
      }
    }
  }
}
message: messages.expected(1)
},
{
code: "a { &:hover { b { top: 0; }}}",

This comment has been minimized.

@jeddy3

jeddy3 Oct 17, 2018

Member

I believe this should be accepted as there is only one level here.

This comment has been minimized.

@inspiratweb

inspiratweb Oct 18, 2018

Contributor

You are right!

a {
  &:hover { // pseudo-class (parent depth level 0 taken)
    b { // nesting level 1
      top: 0;
    }
  }
}
message: messages.expected(1)
},
{
code: "a { &:nest { &:nest-lvl2 { top: 0; .class { bottom: 0; }}}}",

This comment has been minimized.

@jeddy3

jeddy3 Oct 17, 2018

Member

Yes is correctly rejected as there are two levels.

This comment has been minimized.

@inspiratweb

inspiratweb Oct 18, 2018

Contributor

I think the code was wrong, this test should be accepted

a {
  &:hover { // pseudo-class (parent depth level 0 taken)
    b { // nesting level 1
      top: 0;
    }
  }
}
Show resolved Hide resolved lib/rules/max-nesting-depth/README.md
Show resolved Hide resolved lib/rules/max-nesting-depth/README.md
Show resolved Hide resolved lib/rules/max-nesting-depth/README.md
Show resolved Hide resolved lib/rules/max-nesting-depth/README.md
@@ -148,6 +148,65 @@ a {
}
```
### `ignore: ["blockless-pseudoclasses"]`
Ignore pseudoclases that only wrap other rules, and do not themselves have declaration blocks.

This comment has been minimized.

@jeddy3

jeddy3 Oct 17, 2018

Member

Typo: "pseudo-classes"

@jeddy3 jeddy3 changed the title from Blockless pseudoclasses to Add ignore: ["blockless-pseudo-classes"] to max-nesting-depth Oct 17, 2018

@jeddy3

@inspiratweb Thanks for updating the PR.

I think there's been a slight misunderstanding, though.

Sorry I c&p the at-rules block, I have to remove that description line.

I was advocating for keeping the description and updating the behaviour of the code to match it. I think having the blockless-at-rules and blockless-pseudo-classes options behave consistently would be the best user experience. Both should only ignore their respective constructs if they are blockless i.e. without declarations. For example:

a {
  &:hover { /* ignored as blockless */
    b { /* 1 */
      top: 0;
    }
  }
}
a {
  &:hover { /* 1 - counted as has a declaration block */
    top: 0;
    b { /* 2 */
      top: 0;
    }
  }
}

I believe we'll want the node.every(child => child.type !== "decl" conditional to apply to both options. Does that make sense?

Alternatively, if your use case it to ignore pseudo-classes outright and regardless of whether they have a declaration block, then your current logic is correct but the option name is misleading. It should be ignore: ["pseudo-classes"] as "blockless" is not a consideration.

Which is your use case?

@inspiratweb

This comment has been minimized.

Contributor

inspiratweb commented Oct 19, 2018

Yes, it was a mistake naming it blockless, the second option is what i tried to do.
ignore: ["pseudo-classes"] LGTM

@jeddy3

the second option is what i tried to do.

@inspiratweb Thanks for the clarification. I think we're on the right track now. However, I think we need to consider some edge cases with this option:

  • selector lists
  • descendants

What behaviour would you expect for?:

a {
  &:hover,
  b {
    top: 0;
  }
}
a {
  &:hover b {
    top: 0;
  }
}
a {
  &:hover,
  &:focus {
    top: 0;
  }
}

Right now, all three are considered valid. However, I suspect we'd want to disallow the first two. Do you agree? If so, we should check that all the selectors within both complex selectors and selector lists are pseudo-classes before ignoring the whole rule-set. You'll need use postcss-selector-parser to determine this.

Adding options usually seem trivial at first, but experience has taught us that there are usually edge-cases to consider as CSS is a vast language. Fixing the name or behaviour of an option is typically a breaking change, which is why we try to get both an option's name and behaviour correct the first time around.

@@ -148,6 +148,125 @@ a {
}
```
### `ignore: ["pseudo-classes"]`
Ignore pseudoclases.

This comment has been minimized.

@jeddy3

jeddy3 Oct 20, 2018

Member

Typo: pseudo-classes.

This comment has been minimized.

@inspiratweb

inspiratweb Oct 20, 2018

Contributor

In my first approach the first two examples were allowed but I believe what you say may make sense. I'll think about it a bit more and I'll try to propose a solution for this case.

The main reason for doing this rule was to prevent declarations like...

.a {
  .b:hover {}
}

and allow

.a {
  .b {
    &:hover {}
  }
}

Thanks!

This comment has been minimized.

@inspiratweb

inspiratweb Oct 21, 2018

Contributor

This is how it should work in my opinion

1st example
I agree with you because b selector is not :hover dependent and it's a kind of trick to use it like that. I need to fix this.

2nd example
I think it's the main purpose of this issue, I give you a real example for this use case
http://jsfiddle.net/inspiratweb/r1d6qpne/ in my opinion it works as it should because it's just a selector for that nesting level.

3rd example
It's OK because although they are different selectors, both of them are pseudo-classes.
It works like...

a {
  &:hover {
     top: 0;
  }
  &:focus {
    top: 0;
  }
}

Do you agree with me?

This comment has been minimized.

@jeddy3

jeddy3 Oct 21, 2018

Member

Sounds good to me.

Thanks for working it through!

inspiratweb added some commits Oct 21, 2018

@inspiratweb inspiratweb changed the title from Add ignore: ["blockless-pseudo-classes"] to max-nesting-depth to Add ignore: ["pseudo-classes"] to max-nesting-depth Oct 21, 2018

@jeddy3

@inspiratweb Thanks for making the changes and sorry about the time taking to get back to you. I think this PR is almost ready. I've requested some minor documentation changes (as suggestions so you should be able to adopt them automatically if you approve of them).

Show resolved Hide resolved lib/rules/max-nesting-depth/README.md Outdated
Show resolved Hide resolved lib/rules/max-nesting-depth/README.md Outdated
}
```
```css

This comment has been minimized.

@jeddy3

jeddy3 Oct 28, 2018

Member

I think we can delete this example.

Show resolved Hide resolved lib/rules/max-nesting-depth/README.md Outdated
Show resolved Hide resolved lib/rules/max-nesting-depth/README.md Outdated
Show resolved Hide resolved lib/rules/max-nesting-depth/README.md Outdated
}
```
```css

This comment has been minimized.

@jeddy3

jeddy3 Oct 28, 2018

Member

I think we can delete this example... it's similar to the previous two.

Show resolved Hide resolved lib/rules/max-nesting-depth/README.md Outdated
Show resolved Hide resolved lib/rules/max-nesting-depth/README.md Outdated
Show resolved Hide resolved lib/rules/max-nesting-depth/README.md Outdated

jeddy3 and others added some commits Oct 28, 2018

Update lib/rules/max-nesting-depth/README.md
Co-Authored-By: inspiratweb <franxo@gmail.com>
Update lib/rules/max-nesting-depth/README.md
Co-Authored-By: inspiratweb <franxo@gmail.com>
Update lib/rules/max-nesting-depth/README.md
Co-Authored-By: inspiratweb <franxo@gmail.com>
Update lib/rules/max-nesting-depth/README.md
Co-Authored-By: inspiratweb <franxo@gmail.com>
Update lib/rules/max-nesting-depth/README.md
Co-Authored-By: inspiratweb <franxo@gmail.com>
Update lib/rules/max-nesting-depth/README.md
Co-Authored-By: inspiratweb <franxo@gmail.com>
Update lib/rules/max-nesting-depth/README.md
Co-Authored-By: inspiratweb <franxo@gmail.com>
Update lib/rules/max-nesting-depth/README.md
Co-Authored-By: inspiratweb <franxo@gmail.com>
@inspiratweb

This comment has been minimized.

Contributor

inspiratweb commented Oct 28, 2018

Perfect! suggestions applied :D

@jeddy3

jeddy3 approved these changes Oct 29, 2018

@inspiratweb Thanks! LGTM.

@hudochenkov

Thank you!

@hudochenkov hudochenkov merged commit 67596f0 into stylelint:master Nov 1, 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 increased (+0.05%) to 96.333%
Details
@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Nov 1, 2018

  • Added: ignore: ["pseudo-classes"] to max-nesting-depth (#3724).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment