Add stylelint CSS linting - #12934
Conversation
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
|
Nice, given the new |
|
The first run of the css linter job on this PR reports 2746 errors while locally I get 2823 errors. Will look into it. At a first check it appears some files aren't scanned in the job:
|
9d6e06f to
f14b5fb
Compare
|
The bundled themes The one for
Although there are working forks of the Even after making them work, the linting for both themes reports several errors, mostly because of outdated configuration:
It appears the linting scripts of these two themes haven't been used for a long time. Likely, they have been intensively used during the themes development but now they are way behind. Also, bundled themes are part of Core. To me, it makes sense to have a ceentralized tool to lint everything. In the latest commit I added a As said earlier, for now it's best to focus on the functionality. The set of rules can be refined later. It will need some adjustments as some rules aren't applicable in Core, for example |
c0f141a to
b3c8e48
Compare
|
Regarding the a11y rules that were used in twentytwenty, I tried to re-add them to the Core Stylelint configuration by using the compatible stylelint-a11y plugin fork. It's a useful experiment to check a few CSS patterns that are potentially harmful for accessibility:
Among other rules provided by the plugin, this one might be useful: media-prefers-reduced-motion I would suggest to experiment these rules at a later stage, after the initial configuration is proved to be stable and reliable, |
979e0b5 to
e32b4a3
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
683bbba to
151f05e
Compare
desrosj
left a comment
There was a problem hiding this comment.
Thanks for all of the work on this so far, @afercia!
This PR is getting to be quite a beast. In the interest of making this easier to review, could you chunk this out a bit differently? I am thinking that starting with one PR per theme could chunk it down considerably.
Also, I'd love to find ways to utilize the related scripts already maintained within gutenberg, expanding if necessary rather than adding new scripts in this repository that just contributes to the divide. I also think it's worth considering whether splitting the lint related code out into it's own package is more beneficial instead of keeping it bundled in wp-scripts with a bunch of other unrelated things.
There was a problem hiding this comment.
I don't know that I agree with removing the Stylelint-related files from default themes. If anything, I think I would advocate for updating them or adding them to themes without any.
Even though they are maintained in wordpress-develop, that may not always be the case.
Also, the themes are also the most useful when the tooling is self contained. So when someone has a copy of the theme, they should be able to run all of the related tooling when removed from wordpress-develop.
I know that this will potentially be a pain to maintain. But could we make better use of the related tooling available through the [@wordpress/stylelint-config](https://www.npmjs.com/package/@wordpress/stylelint-config) package?
There was a problem hiding this comment.
The whole themes thing is something I wanted to try but I'm not fully convinced about it as well.
One one hand, it appears the bundled themes are not maintained as one would hope. As I mentioned earlier, the Stylelint implementation for twentytwenty is completely not functional.
On the other hand, I would consider the bundled themes no longer in active development. As such, I'm not sure they still need their own custom Styelint configuration. Personally, I think bundled themes are part of Core. Ideally, they should use the same centralized configuration with the same ruleset.
I can think of two options:
- Remove any change to the themes. But then, we should make sure the Bundled themes team actively maintain them. If we go with this route, we are also making a decision that the Bundled themes tooling is not part of Core. It's self contained, as you said, and it can also use different rules (which I would argue upon). As such it should not be part of this PR.
- The other route would be making the decision that from now on, all Bundled themes tooling is part of Core and they should use the centralized tools provided by Core. As such, Bundled themes should be developed and maintained in wordpress-develop. Developers who want to work on a copy of a Bundled theme should develop on wordpress-develop as well. Of course, this option would prevent a lot of code duplication and maintenance cost.
I understand the decision between these two options would require some more broader discussion so that I'd tend to think all changes to the themes should be removed from this PR. Thoughts welcome.
|
|
||
| - name: Set up Node.js for themes needing minification | ||
| if: matrix.theme == 'twentytwentytwo' || matrix.theme == 'twentytwentyfive' | ||
| if: matrix.theme == 'twentytwentytwo' || matrix.theme == 'twentytwentyfive' || matrix.theme == 'twentytwentyone' |
There was a problem hiding this comment.
| if: matrix.theme == 'twentytwentytwo' || matrix.theme == 'twentytwentyfive' || matrix.theme == 'twentytwentyone' | |
| if: contains( fromJSON('["twentytwentyfive", "twentytwentytwo", "twentytwentyone"]'), matrix.php ) |
This makes it much easier to see what the required criteria is. I'm on the fence here about whether we should actually move this to the strategy.matrix instead, though. It would become something like:
strategy:
fail-fast: false
matrix:
theme: [
'twentytwentyfive',
'twentytwentyfour',
'twentytwentythree',
'twentytwentytwo',
'twentytwentyone',
'twentytwenty',
'twentynineteen',
'twentyseventeen',
'twentysixteen',
'twentyfifteen',
'twentyfourteen',
'twentythirteen',
'twentytwelve',
'twentyeleven',
'twentyten'
]
include:
- theme: 'twentytwentyfive'
requires-minification: true
- theme: 'twentytwentytwo'
requires-minification: true
- theme: 'twentytwentyone'
requires-minification: true
Then the conditional check would simply be:
if: matrix.requires-minification
There was a problem hiding this comment.
Needs a decision on the two options for the themes first.
|
I would appreciate some feedback on the threshold mechanism implemented for the rule This rule, accordingly to the CSS coding standards, flags all the over-qualified selectors like, for example:
These are all violations. The problem with it is that there are 1815 such violations. We can't change these selectors otherwise we would trigger a storm of CSS-specificity issues all over. That's the reason why I set them to 'warning'. However, I also want this type of violation to not increase in the future. So the idea is:
|
|
Regarding this point:
I would have loved that too. That's why I started a conversation on a new Gutenberg issue. Looks like things aren't as ideal as necessary to do that. You can read the relevant parts of the conversation starting from this comment.
Thats what this PR does. It uses |
|
The following is a recap of the differences in the Stylelint configuration between the one in this PR for Core and the one used here in Gutenberg. Note that, as I learned from this comment WordPress/gutenberg#81349 (comment) it appears that in Gutenberg different packages may use different rulesets so the comparison can't be fully accurate as on Gutenberg it varies depending on the analyzed package. Methodology: Commands used:
|
151f05e to
7ca37e0
Compare
Trac ticket: https://core.trac.wordpress.org/ticket/29792
Work In Progress (WIP) to add Stylelint CSS coding standards rules enforcement to Core.
Documenting the work done so far:
Core
Two new npm scripts are added together with a Grunt task that can be run individually and is also part of
grunt precommit:css:Additionally, two more scripts are added to try a 'warnings thresholds' mechanism:
This 'warnings thresholds' mechanism is meant to keep the warnings number under control and avoid it increases over time.
The linting rules are defined in the
.stylelintrc.jsfile in the root of the project.Paths and files to be excluded are defined in
.stylelintignore.The themes directory is excluded.
Themes
Some bundled themes already have their own Stylelint scripts:
lint:cssas a standalone script that is not part of the build process. Only for.cssfiles. Useslint-stylewp-script.lint:scssandlint-fix:scssfor the.scssfiles. Plus,build:stylelint, which is part of the build process, for the.cssfiles. All the thre scripts usestylelintdirectly.These existing scripts use their own Stylelint configuration. It made sense when these two themes were under development. I think these should be removed in favor of a centralized Stylelint configuration in core that lints also the themes. This PR adds:
They use the same rules defined in the
.stylelintrc.jsfile in the root of the project.Paths to be excluded are defined in
.stylelintignore-themesinstead.Note on the existing config in Twenty Twenty:
The
lint-stylescript from wp-scripts doesn't walk the directory tree upwards to auto-discover a Stylelint configuration. It expects a configuration in the theme's root. Instead, when used directly, Stylelint does. This doesn't allow to use the Core configuration from the theme.Also, Twenty Twenty uses the
stylelint-a11yStylelint 'plugin' to add two lint rules:If the 'ad-hoc' configurations for the two themes get removed in favor of a centralized configuration, these two a11y ruels would be lost. A decision should be made on whether to add them to the centralized configuration.
Rules
The WordPress CSS Coding Standards (more readable in their GitHub page version) are described in a conversational language style and are difficult to summarize point by point. Extracting some of the most important ones to document the related rules.
The rules configuration extends the rules from the
@wordpress/stylelint-config/scss-stylisticone and add or change rules to cover the following points:One blank line between blocks in a section
Each selector should be on its own line
This was tricky to address as some rules conflict. To make sure indentation linting is performed correctly, a specific order of some rules needs to be preserver.
All properties and values should be lowercase, except for font names and vendor-specific properties
@stylistic/property-casevalue-keyword-casewith exceptions forcurrentColorandoptimizeLegibilityAvoid RGB format
'function-disallowed-list': ['rgb']Line height should also be unit-less, unless necessary to be defined as a specific pixel value.
declaration-property-unit-allowed-listwith'line-height': []Font weights should be defined using numeric values
'font-weight-notation': 'numeric'Refrain from using over-qualified selectors, div.container can simply be stated as .container
Right now, there are 1815 violations of this rule in Core. It's a lot. Things like
input[type="text"]or simplya.currentare considered invalid. As such, I added the ruleselector-no-qualifying-typeand changed the severity type towarning. A decision on how to handle these warnings can be made later.Remove multiple spaces between selector combinators
This is not mentioned in the coding standards but I addressed it anyways to cover cases like
.myclass1 .myclass2 {}The added rule is:
'@stylistic/selector-descendant-combinator-no-non-space': true.Recommended changes to the CSS Coding Standards
Similar to the WordPress PHP Coding Standards for file names, use lowercase and separate words with hyphens when naming selectors. Avoid camelcase and underscores.
Gutenberg already uses a BEM-like naming convention that uses underscores for class selectors. Some new selectors recently introduced in core already follow that convention e.g.:
.wp-tooltip__toggle.On the other hand, there are several violations already in core for both class and ID selectors. A few examples:
.ac_match.privacy_requests#login_error#dashboard_right_now#TB_windowAll of these aren't allowed by the current CSS Coding Standarrds. They can't be changed though, because of backward compatibility concerns.
As such, the current recommendation to use hyphens and avoid underscores isn't applicable.
Add two blank lines between sections
This is not doable with Stylelint. The rule
'@stylistic/max-empty-lines': 1cannot distinguish between nprmal comments and section comments. Two blank lines between sections add little value anyways. Suggest to remove this point from the Coding Standards.Long comments should manually break the line length at 80 characters.
@stylistic/max-line-lengthcan be used to set a global maximum line length. It cannot distinguish between lines of code and lines of comments. Either we set a global 80 characters limit or we should use another tool e.g. a custom postcss script to apply the line length limit only to comments. A little overkill to me. We could just decide to not lint it or change this point in the Coding Standards.Property Ordering
All the points in this sections are not enforceable with Stylelint. They can be kept as genereal recommendation but they will always be subject to personal preferences.
Testing
Don't forget to run
npm installbefore testing.For now, it is important to check the following:
npm run lint:cssandgrunt lint:css.Rather than running the linting on the whole codebase with
npm run lint:cssornpm run lint:css:fixI suggest to also start with lintin single files so to get a clearer view of how it works. Start with a small file, for example:npx stylelint src/wp-admin/css/color-picker.cssThen, run the inter with the auto-fix flag:
npx stylelint src/wp-admin/css/color-picker.css --fixThen, try larger files, for example:
As of Core revision 63292, the Stylelint scan reports the following:
Most of these errors are trivial fixes. Running the auto-fix will reduce the errors to 646. They will need to either be manually fixed or have a
stylelint-disablecomment when appropriate.Use of AI Tools
AI assistance: Yes
Tool(s): GitHub Copilot
Model(s): Claude Sonnet 5.
Used for: Plan and make the initial warnings threshold mechanism.
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.