-
Notifications
You must be signed in to change notification settings - Fork 3
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
Conversation
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! |
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 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 |
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 |
@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. |
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). |
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 |
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? |
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 |
When I tried this PR locally, I personally found it confusing to have two namespaces linted while others are ignored. |
Ok 🤷♂ |
#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:
but it does not give you an error if you do this:
because
.is-
is on the ignore-list for selectors. I could not find a better way to do this without forkingpostcss-bem-linter
.This also only lints selectors that are in
components
orutilities
Testing
components
andutilities
folders