smashingboxes / web-boilerplate

A template for new front-end projects.
3 stars 2 forks source link

Add a CSS linter rule for disallowing & for concatenation #60

Closed dkniffin closed 6 years ago

dkniffin commented 7 years ago

Why?

What changed?

dkniffin commented 7 years ago

Also, for reference, here's the set of rules I was using to test with. The ones that say "valid" in them are fine. The ones with "invalid" will fail this linting rule


  .valid {
    top: 1px;
  }

  &.valid {
    top: 1px;
  }

  &--invalid {
    top: 1px;
  }

  &-invalid {
    top: 1px;
  }

  &invalid {
    top: 1px;
  }

  &_invalid {
    top: 1px;
  }

  &::before { /* This one is valid. Apparantly ::valid isn't a real pseudoselector ¯\_(ツ)_/¯ */
    top: 1px;
  }

  &[valid] {
    top: 1px;
  }

  .valid &,
  .continued & {
    top: 1px;
  }

  .valid + & {
    top: 1px;
  }
taralmaxwell commented 7 years ago

I, personally, really like this rule, @dkniffin ! I have never liked using & for concatenation in CSS. Nice addition to our linter!

jahammo2 commented 7 years ago

Let me say that I like using & for concatenation versus the example Zach mentioned above. However, what I like even more is consistency so I'm okay with this change.

zachary-kuhn commented 7 years ago

Can we add a few other tests that I'd like to make sure work? Would like your opinion on, too.

&.valid.also-valid { /* when both classes are also on the element */
  top: 1px;
}

& .valid,
& .continued { /* specifying multiple descendants */
  top: 1px;
}

& + .valid { /* specifying a peer that follows this element */
  top: 1px;
}

& > .valid { /* specifying a child */
  top: 1px;
}

.valid > & { /* specifying a parent */
  top: 1px;
}

&:focus,
&:hover { /* pseudo-class in addition to pseudo-element */
  top: 1px;
}

&:focus,
&--invalid { /* not so fast. Just want to make sure sneaking something like this in the second line still fails the check */
  top: 1px;
}

& div { /* specifying an element */
  top: 1px;
}

These were all I could think of right now, but want your thoughts on if these fit what you want from this change.

leeallen337 commented 6 years ago

@dkniffin Did we ever reach a conclusion on this? I really like the idea of not using & for concatenation.

dkniffin commented 6 years ago

@leeallen337 I never got a chance to test Zach's last suggestion. If you have some time to test it, I suspect they'll work, but I'm not 100% sure. After that, I think this is good to be merged.

dkniffin commented 6 years ago

@leeallen337 I finally got around to updating this. I made some tests on regex101.com, and put a comment to it in the config, which required me to switch to a .js file for the config.

dkniffin commented 6 years ago

@leeallen337 This is blocking something I'd like to do on boxcar, so I'm going to merge it. If there's any more issues you want me to address, let me know and I'll make a separate PR.

Edit: lol, nevermind. I can't merge until you approve it. So I guess I'll wait on my boxcar feature.

leeallen337 commented 6 years ago

This is going to be awesome. Thanks @dkniffin 💯