Disable NestingDepth rule #32
Conversation
alvinsiu
changed the title
There was a problem hiding this comment.
After thinking about this, I'm fine with disabling the nesting depth check since if we keep it in place we're forced to attach explicit classes (instead of relying on nested selectors) which may not be worth it in some cases.
Looking at the sccs code that is tripping over the UnnecessaryParentReference, I don't see the value in relaxing this rule and we shouldn't use & syntax when it's not needed for nested rules. I think allowing it only serves to confuse things for the reader who might wonder why we including the & in some cases where it's not needed and while in other cases we don't bother with it at all.
|
I just reverted the disabling of UnnecessaryParentReference. My style preference is to always include it, but that's just a style preference so I'm happy to go with your decision. |
| enabled: true | ||
| max_depth: 3 | ||
| ignore_parent_selectors: false | ||
| enabled: false |
There was a problem hiding this comment.
How far down do you need to go? I think turning this off is a little heavy-handed.
There was a problem hiding this comment.
My opinion is that there really isn't necessarily a max depth we should use, but this also really depends on what paradigm FF will be using for styling (which I don't think there necessarily is one). If we're using BEM, then I would actually not change this linting rule at all since it allows for flat css structure with a lot less risk of unintentional styling overlaps. But if we're not using BEM (which I don't really see true BEM in the few codebases I looked in), then nesting is critical, especially with such the frontend codebases being so fragmented (a CSS rule in component library can affect a different app, and that most likely will not be caught in QA).
And from my personal experience, most developers aren't super aware of BEM, and even if they are, there's quite a few rules you have to follow fairly strictly to take full advantage of it. This makes me generally avoid requiring the use of BEM by all engineers and instead encourage the use of "componentizing" styling rules by nesting. Does it make the html/styling less maintainable? Probably a little bit, but I think given the primary concern of our software is backend, so I'd rather not add more overhead for people developing HTML/CSS. If our business switches focus to frontend a bit more where we are prioritizing a more consistent styling across our frontends, then I think that's a good time to revisit.
So for me, it's kind of all or nothing - keep the rule and require BEM (or similar technique), or get rid of the rule and encourage componentizing through nesting for custom styling.
scss-lint.yml
Outdated
| @@ -160,8 +158,7 @@ linters: | |||
| allow_element_with_id: false | |||
|
|
|||
| SelectorDepth: | |||
| enabled: true | |||
| max_depth: 3 | |||
| enabled: false | |||
There was a problem hiding this comment.
I'm not familiar with this rule so I'll defer, but I'd ask if increasing it is better than removing it?
There was a problem hiding this comment.
This rule is similar to NestingDepth, except this is simply for a single selector (so .one .two .three is 3 depth, and .one .two .three .four is 4).
Now that I'm thinking about it more, we may not necessarily want to disable this. If we're trying to do some more complex styling where depth of applicability will matter, then this rule gets in the way, but that's likely more the exception than than norm. And from my point above, frontend software is not the primary focus of our company, so we should pick rules that help suggest better practices for those less familiar, and those more familiar or writing styling intentionally in some way can just mark things as wontfix.
Thanks for asking!
alvinsiu
changed the title
Two primary reasons people cite for having the nesting rules are:
For the first point, while this is true, the having too flat of selector hierarchy can lead to lots of unintentional overlap of styling rules due to overlapping selectors. Since stylesheets are global, there's no guarantee that a selector one developer uses will not affect the styling in a completely different part of the codebase (this is incredibly difficult to test). Using nested selectors where you are more specific about what you're modifying reduces side effects in styling. Removing this rule is not suggesting that we use the most specific selectors at every point possible - it's just removing the suggestion that nesting is always bad practice.
For the second point, this is true, however, the performance differences are pretty negligible. With modern browsers and tooling, this should not be the primary concern of developers, especially since collecting styling is such a tiny fraction of the overal render time.
Also removing the rule that prevents the use of
&when it's not strictly necessary. Enforcing this rule would require developers to understand when it is necessary vs when it is optional. Oftentimes, it can provide more clarity when reading sass to add the optional&. Removing this rule is not suggesting that it should always be used, only that we don't enforce its removal and allow developers the freedom to use it should they feel it adds clarity.