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 stylelint for SCSS linting #8647

Merged
merged 2 commits into from
Aug 8, 2018
Merged

Add stylelint for SCSS linting #8647

merged 2 commits into from
Aug 8, 2018

Conversation

noisysocks
Copy link
Member

@noisysocks noisysocks commented Aug 7, 2018

Supersedes #1151. Closes #8618.

Adds the npm run lint-css and npm run lint-css:fix commands for SCSS linting, and configures npm run lint to automatically run npm run lint-css.

Linting is performed by stylelint. I've configured stylelint to extend stylelint-config-wordpress which enforces the WordPress CSS coding standards.

To save time and reduce merge conflicts, I've disabled every rule that isn't totally trivial to fix (e.g. with a find and replace). We can work towards re-enabling these rules one by one over the medium term.

To test:

  1. npm run lint-css
  2. Open the demo post and check for visual regressions
  3. Check the diff and make sure stylelint --fix or one of my regular expressions didn't completely backfire

@noisysocks noisysocks requested review from tofumatt, ntwb and a team August 7, 2018 07:40
@@ -1,9 +1,11 @@
.editor-block-list__block[data-type="core/button"] {

Copy link
Member Author

@noisysocks noisysocks Aug 7, 2018

Choose a reason for hiding this comment

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

Weird... why does it want empty lines here, @ntwb?

Copy link
Member

Choose a reason for hiding this comment

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

A few reasons:

• Due to the number of permutations and combinations of where and where not new empty lines should occur, I've an issue to audit all of these at WordPress-Coding-Standards/stylelint-config-wordpress#65

• The lack off SCSS used currently in WordPress leading to not having an official "WordPress SCSS Coding Standards"


Solution/Workaround:

Update the config used here to override the appropriate rules here in this PR and then follow up by updating stylelint-config-wordpress, either or both the CSS and SCSS configs and releasing a new version of stylelint-config-wordpress.

@noisysocks noisysocks added the [Type] Code Quality Issues or PRs that relate to code quality label Aug 7, 2018
Copy link
Member

@ntwb ntwb left a comment

Choose a reason for hiding this comment

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

Awesome work @noisysocks 💯

color(theme(primary) shade(30%)) 72%,
theme(primary) 72%
) !important;
/* stylelint-enable */
Copy link
Member

Choose a reason for hiding this comment

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

Note to self: Investigate supporting this in stylelint core

#poststuff h2.hndle { /* WordPress selectors yolo */
#poststuff h2.hndle {

/* WordPress selectors yolo */
Copy link
Member

Choose a reason for hiding this comment

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

This is not ideal, it's an idiosyncratic stylelint issue that requires fixing 😞

@ntwb
Copy link
Member

ntwb commented Aug 7, 2018

Reviewing the diff without whitespace changes is helpful 👌

@gziolo
Copy link
Member

gziolo commented Aug 7, 2018

Any idea why one of the unit tests failed on Travis?

@ntwb
Copy link
Member

ntwb commented Aug 7, 2018

A snapshot failure: https://travis-ci.org/WordPress/gutenberg/jobs/412993016#L5006

in file packages/editor/src/components/default-block-appender/test/index.js:47:21

A e2e test failure: https://travis-ci.org/WordPress/gutenberg/jobs/412993019#L1286

FAIL test/e2e/specs/meta-boxes.test.js
Error: Navigation Timeout Exceeded: 30000ms exceeded

Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

All of those weird new empty newlines aren't ideal, but I prefer automated, written rules I don't like over unwritten, unautomated ones I do like.

I'd be cool to fix all those newlines (eg https://github.com/WordPress/gutenberg/pull/8647/files#diff-44e5de5ab7a2465d8a970cbd777c427eR2). But hey, rules are cool!

@noisysocks
Copy link
Member Author

Thanks all! I disabled comment-empty-line-before, rule-empty-line-before, and at-rule-empty-line-before as quick fix for the weird whitespace that was being added. Better to have the bare minimum than nothing at all 🙂

@noisysocks
Copy link
Member Author

noisysocks commented Aug 8, 2018

screen shot 2018-08-08 at 15 01 15

That ain't right... 😅

edit: Fixed thanks to #8660

@noisysocks noisysocks force-pushed the add/stylelint branch 3 times, most recently from 4d71a48 to 061cc48 Compare August 8, 2018 07:17
Adds stylelint and configures it to lint SCSS files with a very
conservative rule set.
package.json Outdated
@@ -161,11 +163,13 @@
"fixtures:server-registered": "docker-compose run -w /var/www/html/wp-content/plugins/gutenberg --rm wordpress ./bin/get-server-blocks.php > core-blocks/test/server-registered.json",
"fixtures:generate": "npm run fixtures:server-registered && cross-env GENERATE_MISSING_FIXTURES=y npm run test-unit",
"fixtures:regenerate": "npm run fixtures:clean && npm run fixtures:generate",
"lint": "npm run lint-js && npm run lint-pkg-json",
"lint": "npm run lint-js && npm run lint-pkg-json && npm run lint-css",
Copy link
Member

Choose a reason for hiding this comment

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

Can we run it concurrently?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 b977145

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Awesome one 👏

I ❤️ npm run lint-css:fix the most!

@noisysocks noisysocks merged commit 8cbc988 into master Aug 8, 2018
@noisysocks noisysocks deleted the add/stylelint branch August 8, 2018 07:39
@ntwb
Copy link
Member

ntwb commented Aug 8, 2018

❤️

@aduth
Copy link
Member

aduth commented Aug 8, 2018

In case anyone is like me and was looking for editor integration for Sublime Text, here's the plugin for use with SublimeLinter:

https://github.com/SublimeLinter/SublimeLinter-stylelint

@youknowriad
Copy link
Contributor

Who uses sublime nowadays :trollface: ?

@tofumatt
Copy link
Member

tofumatt commented Aug 8, 2018

It's way faster than Atom! And it never leads to wq in my commit messages. :trollface: :trollface: :trollface:

@youknowriad
Copy link
Contributor

@tofumatt I fully deserve this one :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use ::before (pseudo elements) instead of :before (pseudo classes)
6 participants