Skip to content
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 css selector bem linting #554

Closed
wants to merge 2 commits into from
Closed

Add css selector bem linting #554

wants to merge 2 commits into from

Conversation

calebeby
Copy link
Member

#63 Overview

Closes #517
I had to pass a lot of options to https://github.com/postcss/postcss-bem-linter in order for it to accept our class naming convention. There easily could have been things that I missed.

postcss-bem-linter does not want to accept non-component classes with prefixes like .is-open, .t-dark, etc. The only way I could figure out how to make it allow those was to ignore selectors with those entirely.

So it will give you an error if you do this:

// components/button.scss
.button {}
// ^ should be .u-button, will error

but it does not give you an error if you do this:

// components/button.scss
.button.is-open {}

because .is- is on the ignore-list for selectors. I could not find a better way to do this without forking postcss-bem-linter.

This also only lints selectors that are in components or utilities

Testing

  1. Create components and utilities folders
  2. Add files to those folders
  3. Add incorrect selectors and make sure it flags them
  4. Add correct selectors and make sure it doesn't flag them

@calebeby calebeby requested review from a team March 18, 2020 18:20
@tylersticka tylersticka self-requested a review March 18, 2020 19:35
@tylersticka
Copy link
Member

I plan to take a closer look at this as soon as I'm able to. I encourage others to provide feedback before then, but I'd prefer not to merge this till I've had a chance to dig in. Thanks!

@tylersticka
Copy link
Member

tylersticka commented Mar 19, 2020

I think you've done the best job you can with this, @calebeby. But I'm wondering if the PostCSS BEM Linter just isn't ready for this convention yet.

In my ideal world, we'd be able to /** @define objects */ or /** @define themes */ and have the linter treat those namespaces with the same detail as /** @define utilities */ or whatever. But as-is, we're sort of just linting two namespaces and ignoring everything else.

To be clear, I don't think this is your fault… it's clear from other issues like postcss/postcss-bem-linter#117 that this is a known limitation. But I fear implementing a partial solution like this makes it less intuitive to understand how conventions are enforced, and may give us a sense of false confidence depending on where the bulk of our styles end up living.

I've messaged Harry Roberts on Twitter in case he's aware of alternatives: https://twitter.com/tylersticka/status/1240658351819640832

@calebeby
Copy link
Member Author

calebeby commented Mar 19, 2020

Yeah, that's kind of where I landed too, it's really not something the library is designed to do, what I have is not ideal.

If you want, I could fork it and add that functionality

@tylersticka
Copy link
Member

@calebeby My knee-jerk reaction to maintaining a fork of a BEM selector linting plugin is negative, unless it's comparable in complexity to something like our lint config repos.

Or if we could propose it as a PR, I suppose that'd put it outside the realm of our responsibility.

I dunno, I'm torn. Would love @spaceninja's opinion on this.

@spaceninja
Copy link
Member

It depends on how open the maintainers are to us submitting a PR.

If it seems like there's a good chance we could do the work and get it merged into the main repo, then game on!

But if it seems like it would languish for a long time, it might be more hassle than it's worth.

FWIW, I've never worked on a project with class name enforcement in linting, it was just understood that contributors should follow BEM rules, and if/when someone slipped up, we'd catch it in code review. As a result, I'm personally not super invested in this aspect of linting, but if the team wants it or sees value, I'm not opposed. (Just to explain why I view the cost/benefit of maintaining a fork this way).

@calebeby
Copy link
Member Author

FWIW, I've never worked on a project with class name enforcement in linting, it was just understood that contributors should follow BEM rules, and if/when someone slipped up, we'd catch it in code review.

I like this. I feel like this PR gets us 80% of the way there to get automated catches for this kind of thing, but implementing the ability for it to catch the remaining 20% would take a lot more time, so I don't think it is worth it. I think that human reviewers can catch those cases

@tylersticka
Copy link
Member

I've encountered many cases (both on internal and client-facing projects) where automated selector linting came in handy, or when it was added later and flagged dozens of missed issues. Based on those experiences, I'm not sure I'd agree that human review is an adequate substitute.

That said, I feel more confident in our ability to catch those issues manually than I do our ability to consistently maintain a linting plugin. So I think we should close this PR and revisit the issue at a later date.

Sound good?

@calebeby
Copy link
Member Author

I think that what we have in this PR is better than nothing. I think we should merge it. Like you said, later on we can revisit

@tylersticka
Copy link
Member

When I tried this PR locally, I personally found it confusing to have two namespaces linted while others are ignored.

@calebeby
Copy link
Member Author

Ok 🤷‍♂

@calebeby calebeby closed this Mar 23, 2020
@spaceninja spaceninja deleted the css-selector-linting branch March 11, 2021 00:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add CSS selector linting
3 participants