no-mergable-selectors should not raise warning with @-blocks or mixins

[BPScott]

Currently the no-mergable-selectors is very aggressive at trying to merge selectors, going so far as to claim that items within @media and mixins blocks must be moved into each selector. For instance it raises warnings when writing:

.item {
  width: 100%;
}

.item__inner {
  width: 75%;
}

@media (min-width: 50em) {
  .item {
    width: 50%;
  }

  .item__inner {
    width: 50%;
  }
}

As it prefers merging those .item and .item__inner selectors, suggesting that you should write it like:

.item {
  width: 100%;

  @media (min-width: 50em) {
    width: 50%;
  }
}

.item__inner {
  width: 75%;

  @media (min-width: 50em) {
    width: 50%;
  }
}

In my opinion the first example is just as valid, and is how I prefer to write my scss, as I find logically grouping by viewport size makes more sense. I actually use a mixin for outputting my media queries so replacing the @media blocks with @includes should also not raise warnings.

To reproduce this, add the following to the bottom of no-mergable-selectors.scss:

// make sure a selector inside a media query is not marked as mergable
@media (min-width: 50em) {
  h1 {
    padding: 0;
  }
}

// make sure a selector inside a supports query is not marked as mergable
@supports (display: flex) {
  h1 {
    display: flex;
  }
}

// make sure a selector inside a mixin is not marked as mergable
@include mq($min: 50em) {
  h1 {
    border: 0;
  }
}

I would expect this to raise no additional linting errors.

[DanPurdy]

Hmm, i'm not sure. I feel like the first example is a bad way of writing media queries. The second example is much clearer and easier to follow from a developer experience point of view. Having a block defined once with all contexts contained within is much clearer than duplicating your selectors within a media query context.

Technically it is a duplicated selector, it's still targeting the same element but under a certain context. I'm open to discussion here though. I think it's probably another case where we may have become a little too stylistic in our reach..

[BPScott]

The second example is much clearer and easier to follow from a developer experience point of view. Having a block defined once with all contexts contained within is much clearer than duplicating your selectors within a media query context.

I disagree with this. Where there are many selectors and contexts, I find having single contexts and duplicated selectors easier to grok that the alternative. This is because mostly I tend to care about sets of selectors as related to each other, and I mostly think about understanding an overview of all items within a given context, rather than focusing in on a single element and thinking "what does this selector look like in all contexts".

But this discussion isn't about convincing each other on which nesting style is better :) They're both equally valid in my opinion. I think scss-lint is being too heavy handed currently as it is saying "The way you write media queries in native CSS is bad and must be changed". This feels very hostile to people who are comfortable writing CSS but are new to Sass. Forcing people to use Sass features sounds too opinionated for my liking.

I do appreciate that "use root level @ blocks" or "use nested @ blocks" is a stylistic choice within a codebase and sass-lint should allow users to choose one of those options, but I think that choice is distinct from the no-mergable-selectors rule.

How about this...

The notion of "mergable selectors" should take into account the context (i.e. @media, @supports or mixin) they are written in. Both of my examples in the original issue should be valid when used in isolation when only the no-mergable-selectors rule is enabled. However if those two styles are mixed and matched sass-lint should raise a warning. e.g.

.item {
  width: 100%;

 @media (min-width: 50em) {
    background-color: red;
  }
}

@media (min-width: 50em) {
  .item {
    width: 50%;
  }
}

should emit a warning stating that the two ".item when the viewport width is >=50em" rules should be merged.

In addition to that there can be a new rule, something like root-level-conditional-groups (as the spec claims they're called with an option called style, that would allow the user to choose if they want to force these blocks to only exist within a selector (your preferred style), or if they should always exist at the root.

In that world, personally I would enable no-mergable-selectors, but disable root-level-conditional-groups as I want to catch cases like .foo { color:red; } .foo { background-color: blue; }, but believe there are valid use cases for both styles of conditional group nesting within my codebase.

[DanPurdy]

Yeah I agree, that's one thing I really want to avoid with sass-lint is it becoming too opinionated. It should be a tool to help everyone enforce their own opinions! 😄

Our whole mindset with sass-lint is for small single purpose rules so it's definitely the right approach to split these ideas up. If you'd like to create a separate issue for the new rule proposal so it doesn't get lost here then that would be appreciated.

As for enhancing the no-mergeable-selectors rule I'll look into it hopefully for 1.4.0

[Andersos]

Had to change my styles because of this. My two cents. I didn't feel my old code was wrong even thought the no-mergeable-selectorssays so. See the point about reducing selectors ("column" in this example). Agree with @BPScott that forcing users of this rule to nest queries is very opinionated. Maybe an option to ignore @media and @include is sufficient? In the end I had no real problems with having to change my code after understanding what it wanted.

Old

@media all and (min-width: 600px) {
    %column {
        float: left;
    }
}

%column {
    padding-right: 1rem;
    padding-left: 1rem;
}

New

%column {
    padding-right: 1rem;
    padding-left: 1rem;

    @media all and (min-width: 600px) {
        float: left;
    }
}

[intellix]

in regards to the technically a duplicated selector, this bourbon keyframes mixin also throws, which shouldn't be merged at all:

@include keyframes(fade-in) {
    0% {
        opacity: 0;
    }

    100% {
        opacity: 1;
    }
}

@include keyframes(fade-out) {
    0% {
        opacity: 1;
    }

    100% {
        opacity: 0;
    }
}

With the following sort of error:

  12:5   warning  Rule `0` should be merged with the rule on line 2    no-mergeable-selectors
  32:5   warning  Rule `100` should be merged with the rule on line 6  no-mergeable-selectors

[DanPurdy]

Thanks @intellix yeah, I'm currently redoing this rule to take into account all mixins. It will warn you if a selector can be merged within a mixin but wont attempt to warn you across mixins as I think we open up a whole can of worms there, maybe something for the future though.

Hoping to have this sorted soon but I've just not had the time in the past few days to work on this.

[mischkl]

@DanPurdy that's awesome that you're taking mixins into account; in addition we also would like to consolidate all styling within a specific media query, so we would also like at least the option to disable the behavior of suggesting merging media queries into other selectors (as @BPScott also describes). Please don't close this issue until that behavior is also implemented, or at least a follow-up issue is created. ;) Thanks for the awesome work! :)

[danimalweb]

+1 for this.

Just had the same warning for:

@keyframes Before {
  50% {
    transform: rotate(0deg);
  }

  100% {
    transform: rotate(45deg);
  }
}

@keyframes After {
  50% {
    transform: rotate(0deg);
  }

  100% {
    transform: rotate(-45deg);
  }
}

[alfaproject]

+1 for same use case as @danimalweb

[eugef]

I am aslo following the approach described by @BPScott and I think that element can be duplicated in different @-blocks. So for now no-mergeable-selectors rule is useless for me.

Would be nice to have an option to allow duplication over different @-blocks.

[DanPurdy]

Hi @eugef I actually meant to label this as a bug after our discussion above. It should be doing what it does but I just haven't had the time recently to get round to fixing it 😢 soon though!

[DanPurdy]

So I'm handling this with the rule re write for the 1.6 release and the gonzales 3.2.x update. It'll still be look in every node but should be so aggressive with what it tries to merge. I'm sure with feedback we can fine tune it to a level that suits everyone.

[DanPurdy]

@BPScott @alfaproject @eugef @danimalweb @intellix I've just tested all your examples again with the new mergeable selectors rule I added to 1.6.0 and they all work as discussed! I'll close this issue but if you find anything else then please raise another issue and enjoy 1.6!