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

stylelint-config: the selector-class-pattern rule rejects some default block, widget, and editor classes #28616

Open
andronocean opened this issue Jan 30, 2021 · 13 comments · May be fixed by #28954
Assignees
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues. Good First Issue An issue that's suitable for someone looking to contribute for the first time [Package] stylelint config /packages/stylelint-config [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.

Comments

@andronocean
Copy link

Description

The stylelint-config package sets a selector-class-pattern rule to enforce kebab-case CSS class names. It looks like the regex was taken straight from stylelint's docs:

'^([a-z][a-z0-9]*)(-[a-z0-9]+)*$',

This ends up flagging a lot of BEM-style default Gutenberg classes like wp-block-media-text__content and wp-block-button__link, editor class versions, as well as old widget classes like widget_recent_entries. For themes and plugins that want to create custom styles for these elements, this adds a lot of noise in the stylelint output.

This issue was brought up in the old stylelint-config-wordpress repo, but got closed without resolution when the project was migrated here.

It seems like a WP-focused ruleset ought to accept WP-generated classes, so it would be good to either make the rule a little more comprehensive, or give some guidance in the package README about overriding it to target block/editor/widget classes.

Reproducing the error

  1. Create a new npm project installing stylelint and @wordpress/stylelint-config as devDependencies, and a "stylelint" key in package.json. (Example below for reference.)
  2. Create a CSS file in the same project using block, editor, or widget classes like above.
  3. Run stylelint (npx stylelint "*.css".) It will complain about the class names containing underscores.

Code snippets

Example css (from TwentyTwentyOne):

/* stylelint will flag this */
.wp-block-cover .wp-block-cover__inner-container,
.wp-block-cover-image .wp-block-cover__inner-container {
	width: calc(100% - calc(2 * var(--global--spacing-vertical)));
}

.but-this-is-a-totally-fine-style {
	margin: 4px;
}

Example package.json:

{
  "name": "wp-stylelint-config-bug",
  "version": "1.0.0",
  "devDependencies": {
    "@wordpress/stylelint-config": "^19.0.0",
    "stylelint": "^13.9.0"
  },
  "stylelint": {
    "extends": "@wordpress/stylelint-config",
    "rules": {}
  }
}
@andronocean andronocean changed the title stylelint-config: the selector-class-pattern rule some block, widget, and editor classes stylelint-config: the selector-class-pattern rule rejects some default block, widget, and editor classes Jan 30, 2021
@gziolo gziolo added [Package] stylelint config /packages/stylelint-config CSS Styling Related to editor and front end styles, CSS-specific issues. Needs Dev Ready for, and needs developer efforts Good First Issue An issue that's suitable for someone looking to contribute for the first time labels Jan 31, 2021
@gziolo
Copy link
Member

gziolo commented Feb 1, 2021

This rule is disabled in Gutenberg which only confirms that it is too strict:

"selector-class-pattern": null,

It should allow one occurrence of __.

@rafaelgallani
Copy link
Contributor

@gziolo Can I work on this one?

@gziolo
Copy link
Member

gziolo commented Feb 7, 2021

Sure, you can always pick an issue that isn't marked as in progress or it doesn't have a PR referenced.

@rafaelgallani
Copy link
Contributor

@gziolo

It should allow one occurrence of __.

I changed the regex to allow only one occurrence of __element, but there are places where this is happening more than once, like here:

.wp-block-button__inline-link {
color: $gray-700;
height: 0;
overflow: hidden;
max-width: 290px;
&-input__suggestions {

When compiled, this turns into .wp-block-button__inline-link-input__suggestions, which would be warned in the stylelint.
Should I allow more than one __element--modifier, or keep it that way?

@gziolo
Copy link
Member

gziolo commented Feb 8, 2021

Can you update those class names so we could see the number of exceptions that exists? It would help to make the decision.

@rafaelgallani
Copy link
Contributor

rafaelgallani commented Feb 8, 2021

I'm not really sure if I understand it, but I ran the stylelint with both regexes, here are the results for it:

Rule Regex example - link Number of occurrences Stylelint output - Link
Allowing ONLY ONE element + modifier https://regexr.com/5m0g9 738 https://pastebin.com/3Bi95VwV
Allowing MULTIPLE element + modifier repetitions https://regexr.com/5m0gi 734 https://pastebin.com/wyyKKiDH

This is not including the compiled css, only the scss sources.
I can run the linter with the compiled files as well if that's the case.

@gziolo
Copy link
Member

gziolo commented Feb 9, 2021

@rafaelgalani, stellar work. "Allowing ONLY ONE element + modifier" is very close to what we need as explained in:
https://github.com/WordPress/gutenberg/blob/master/docs/contributors/coding-guidelines.md#naming

All class names assigned to an element must be prefixed with the name of the package, followed by a dash and the name of the directory in which the component resides. Any descendent of the component's root element must append a dash-delimited descriptor, separated from the base by two consecutive underscores __.

Root element: package-directory
Child elements: package-directory__descriptor-foo-bar

block__element--modifier - this is probably the only class name from the list of matched elements that shouldn't be there, everything else is good.

@rafaelgallani
Copy link
Contributor

@gziolo
Great! Thanks. 😃 Sorry for taking so long to keep up.
These are the result I got after the changes:

Rule Regex example - link Number of occurrences Stylelint output - Link
Allowing ONLY ONE block + element (without modifier) https://regexr.com/5m7no 737 https://pastebin.com/U3PEAbXm

There's not much difference between the block__element--modifier and the block__element-name results. What do you think?

@gziolo
Copy link
Member

gziolo commented Feb 11, 2021

Yes, I wouldn't expect to see -- in the code at all, but you found some occurrences so those results seem valid. I think you have everything ready to adjust the rule.

@gziolo gziolo added the [Status] In Progress Tracking issues with work in progress label Feb 11, 2021
@rafaelgallani rafaelgallani linked a pull request Feb 11, 2021 that will close this issue
6 tasks
@gziolo gziolo added the [Type] Bug An existing feature does not function as intended label Feb 12, 2021
@Mamaduka Mamaduka added [Type] Enhancement A suggestion for improvement. and removed [Type] Bug An existing feature does not function as intended Needs Dev Ready for, and needs developer efforts labels Jul 19, 2022
JoelCDL added a commit to cdlib/intracdl-ui that referenced this issue Feb 24, 2023
JoelCDL added a commit to cdlib/intracdl-ui that referenced this issue Feb 27, 2023
@michaelw85
Copy link

Maybe this is not restrictive enough for WP but I wanted to share it anyway.
This is the regex I'm currently using in my company codebase to allow BEM selector:

^([a-z][a-z0-9]*)((--?|__)?[a-z0-9]+)*$

@PoliWen
Copy link

PoliWen commented Jul 21, 2023 via email

@PoliWen
Copy link

PoliWen commented Oct 17, 2023 via email

@rune-encoder
Copy link

rune-encoder commented Feb 20, 2024

This seems to work very well for BEM naming conventions for SCSS using stylelintrc.json.

I have these settings set up on my .stylelintrc.json file.

{
  "extends": "stylelint-config-standard-scss",
  "rules": {
    "selector-class-pattern": [
      "^[a-z]([a-z0-9-]+)?(__([a-z0-9]+-?)+)?(--([a-z0-9]+-?)+){0,2}$",
      {
        "message": "Expected BEM naming convention for class."
      }
    ]
  }
}

The regex expression I am using is Potherca BEM Regex

I only removed the /. from the Regex because I noticed when stylelint is run it does not check for the period in class names. It was failing all the classes. Once removed it seemed to read all the classes and pass and fail them well.

Classes Passed:
.block__element
.block__element--modifier
.block-a__element-b--modifier-c

Classes Failed:
.block__element--MODIFIER
.block-a__element--

etc.....

At least this is what I think after testing it out a bit...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues. Good First Issue An issue that's suitable for someone looking to contribute for the first time [Package] stylelint config /packages/stylelint-config [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants