WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.5k stars 4.19k forks source link

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

Open andronocean opened 3 years ago

andronocean commented 3 years ago

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:

https://github.com/WordPress/gutenberg/blob/d9e7541fe29c0022f2f5de2570ec60bee670aede/packages/stylelint-config/index.js#L101

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": {}
  }
}
gziolo commented 3 years ago

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

https://github.com/WordPress/gutenberg/blob/d9e7541fe29c0022f2f5de2570ec60bee670aede/.stylelintrc.json#L11

It should allow one occurrence of __.

rafaelgallani commented 3 years ago

@gziolo Can I work on this one?

gziolo commented 3 years ago

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

rafaelgallani commented 3 years ago

@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:

https://github.com/WordPress/gutenberg/blob/fb1cc0e8cec5c55c17de72b13a13214beb6fe602/packages/block-library/src/button/editor.scss#L36-L42

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 commented 3 years ago

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

rafaelgallani commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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.

michaelw85 commented 1 year ago

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 commented 1 year ago

这是来自QQ邮箱的假期自动回复邮件。   您好,邮件已收到。记得常联系哦

PoliWen commented 1 year ago

这是来自QQ邮箱的假期自动回复邮件。   您好,邮件已收到。记得常联系哦

rune-encoder commented 8 months ago

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...