sds / scss-lint

Configurable tool for writing clean, consistent SCSS
MIT License
3.65k stars 466 forks source link

Expand parent selectors in BemDepth #833

Open otbpvose opened 8 years ago

otbpvose commented 8 years ago

When I enable this rule, it picks up on some instances of the BemDepth being higher than the default limit (of 1, although set it manually as well) but only a small proportion. I'm not getting any errors within the output and it's exiting without an error or warning code.

I'm kinda stumped on what might be causing the issue, I've upgraded to the latest version and tried reducing down to 0 elements as well, which again only shows a proportion of the instances where it should fail.

sds commented 8 years ago

Hey @otbpvose,

It would be very helpful for you to include some concrete examples so it's clear what behavior you are expecting versus what you are observing. For example, one or two SCSS code snippets along with the output you see, versus the output you expect.

Thanks for your help!

otbpvose commented 8 years ago

After writing it out (thanks @sds :D ), have figured out what the issue is, the BemDepth rule only works on fully constructed classes, and won't pick up ones constructed via referencing the parent selector (& tag). So I guess the bug is that?


My config is set to run against: scss_files: 'app/assets/stylesheets/responsive/**/*.*css' and I've activated the rule in the following manner:

linters:
  BemDepth:
    enabled: true

Then when I run the command scss-lint I get the following output:

$ scss-lint
app/assets/stylesheets/responsive/components/flight_result.scss:191:1 [W] BemDepth: BEM selectors should have no more than 1 element, but `flight-result__cta__message` has 2
app/assets/stylesheets/responsive/components/lightbox.scss:77:1 [W] BemDepth: BEM selectors should have no more than 1 element, but `lightbox__content__scroll-fix` has 2

The code starting at 191 (in flight_result.scss) looks as follows:

.flight-result__cta__message {
  display: block;
}

and is wrapped in the following context:

.flight-result {
  ...

  &__messages {
    display: none;

    &:lang(en) {
      @include responsive-max-width-xl {
        background-color: $secondary;
        border-bottom: 3px solid darken($secondary, 5%);
        display: table-cell;
        vertical-align: middle;
        width: 30%;

        .flight-result__cta__message {
          display: block;
        }
      }
    }
  }

  ...
}

While ones it isn't identifying are as follows:

&__reason {
  ...

  &__title {
    ...
  }
}
sds commented 8 years ago

Thanks for clarifying the issue, @otbpvose.

The problem with supporting & parent selector expansion is that can get particularly gnarly with comma selectors, for example:

.one, .two, .three {
  &.a, &.b, &.c {
    &.alpha, &.bravo, &.charlie {
      ...
    }
  }
}

Sass will compile the above into 27 selectors: .one.a.alpha, .one.a.bravo, ..., .three.c.charlie. This makes reporting errors difficult since you have to deduce which sequence of selectors lead to a compiled selector that violated the BEM depth limit, and present a message to the user that helps them understand which sequence lead to the problem. I think you can see why this is difficult.

I'm open to a pull request that addresses this, but won't be implementing this myself.