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 bem selector-class-pattern stylelint rule #8

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

juantxokupul
Copy link
Member

No description provided.

@juantxokupul juantxokupul requested a review from quicoto June 21, 2022 10:54
@quicoto
Copy link
Member

quicoto commented Jun 21, 2022

@juantxokupul Sweet, can we add a test for something like:

.block__elementSomething

Which I've seen in many places. See what happens, or what do we think should happen.

Thank you

@raohmaru raohmaru self-requested a review June 21, 2022 15:37
package.json Outdated
@@ -1,7 +1,7 @@
{
"name": "@netcentric/stylelint-config",
"description": "Netcentric's coding and style rules for Stylelint",
"version": "1.2.1",
"version": "1.3.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Package version is changed automatically after a release, no need to change it manually.
Please revert this change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@raohmaru
Copy link
Contributor

Following contribution guidelines, please fill an issue and link to this PR. This way we can track and discuss this change.

@raohmaru
Copy link
Contributor

In my opinion, we shouldn't force a naming convention. This project is open source and hence potentially it can be used by anyone and not only by Netcentric. And as much as at Netcentric we love BEM, we cannot force it to any developer that wants to use this stylelint configuration.

@quicoto
Copy link
Member

quicoto commented Jun 22, 2022

@raohmaru developers can always overwrite this rule in their stylelint.config (which they'll need anyway to add the extend rule with our package)

Considering this should be one of the standard packages we use for all our new projects, having to copy and paste this long regex every time to follow the NC Code Style seems overkill. Seems more optimal to have it here in the package and remove it in the few cases where it's not wanted.

@juantxokupul juantxokupul linked an issue Jun 22, 2022 that may be closed by this pull request
@quicoto
Copy link
Member

quicoto commented Jun 22, 2022

I liked it when it was a 1 liner in the config.

I'm not a fan of adding a new dependency, which can not be taken out via config when someone includes this package.

@juantxokupul
Copy link
Member Author

Issue created by @prototowb, thank you!

@raohmaru This PR was created since kebab-case was the default class naming convention, and we thought that bem would be better.
I think this repository is intended to establish some default configs. As @quicoto says, you can always override.

@quicoto yes, I also don't like to add a dependency, but you need a css post-processor to support scss nested selectors.

@quicoto
Copy link
Member

quicoto commented Jun 22, 2022

I'm retracting my enthusiasm with this change. I don't think we should go forward with it.

@juantxokupul
Copy link
Member Author

juantxokupul commented Jun 22, 2022

NOTE: you can take out this config setting the rule to null

"plugin/selector-bem-pattern": null

(EDIT) you also need to define the component in order to have this working. Without that, this will do nothing -> no need to override anything.

@juantxokupul juantxokupul force-pushed the add-bem-selector-class-pattern branch from 993e036 to 81d6ee6 Compare June 22, 2022 08:44
@juantxokupul
Copy link
Member Author

juantxokupul commented Jun 22, 2022

PR updated

I realized that stylelint is already a post processor and the only thing needed was to set resolveNestedSelectors option to true

The only advantage I see with the plugin, is that you can apply bem naming convention only in certain css files/sections

@juantxokupul
Copy link
Member Author

juantxokupul commented Jun 22, 2022

@juantxokupul Sweet, can we add a test for something like:

.block__elementSomething

Which I've seen in many places. See what happens, or what do we think should happen.

Thank you

this is allowed and I think it should
I searched in NC wiki and found this https://projects.netcentric.biz/wiki/pages/viewpage.action?spaceKey=~natalia.venditto&title=Code+Style+-+CSS+Guidelines#CodeStyleCSSGuidelines-Namingrules

The page is under 'Archive OLD' itself but seems like a good reference to me

@quicoto
Copy link
Member

quicoto commented Jun 22, 2022

@juantxokupul Please have a look at the updated page (subpages) of https://projects.netcentric.biz/wiki/pages/viewpage.action?pageId=48141171

Which also needs to be updated... Archive pages should not be used.

@juantxokupul
Copy link
Member Author

@quicoto thank you!

Fortunately, it seems the 6. Classes (.name) section is a copy/paste from the old :)

@juantxokupul juantxokupul self-assigned this Jun 22, 2022
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.

Config enforcing kebab-case class names
3 participants