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

CSS: Add package.json for linting and compiling #1159

Closed
wants to merge 7 commits into from

Conversation

m-e-h
Copy link

@m-e-h m-e-h commented Jul 3, 2017

This is obviously a first pass since it hasn't really been discussed or tested beyond my Linux desktop. But I think it could help to guide and keep uniform some of the SCSS/CSS and JS commits.
A package.json would give us an agnostic build process. #1149
It's just node, which is what our CSS/JS development tools are built for.
https://github.com/WordPress-Coding-Standards/stylelint-config-wordpress
https://github.com/WordPress-Coding-Standards/eslint-config-wordpress

According to the NPM docs it serves 3 main purposes:

  1. It serves as documentation for what packages your project depends on.
  2. It allows you to specify the versions of a package that your project can use using semantic versioning rules.
  3. Makes your build reproducible which means that its way easier to share with other developers.

This file could also, I think, take the place of and hopefully allow removal of the .jscsrc and .jshintignore files in the repo. Might be time to switch to ESLint anyway. JSCS was merged into ESLint over a year ago https://core.trac.wordpress.org/ticket/31823

Here's how it works for anybody wanting to submit a CSS pull-request:
From the _s/ root directory in the command line:
first npm install or yarn
then npm run build:css or yarn build:css
And that's it. They've linted the style.css for errors, compiled the SCSS, added prefixes according to the specified browser support, and automatically fixed some errors with the remaining errors printed in the terminal for manual fixing.

You can replace build:css with any of the other "scripts" commands listed in the file.

@grappler
Copy link
Collaborator

grappler commented Jul 4, 2017

I think we should limit the scope to just CSS and JS for this PR but this will allow us to have a standard way of generating RTL styles (see #1154) and the POT file (see #884 (comment)).

stylelint style.css --fix is still an experimental feature and does not yet automatically fix all of the issues in the style.css

If we run CSS Lint on the current style.css

style.css
   6:277  ⚠  Expected line length to be no more than 80 characters            max-line-length
  14:83   ⚠  Expected line length to be no more than 80 characters            max-line-length
  16:81   ⚠  Expected line length to be no more than 80 characters            max-line-length
  52:23   ⚠  Expected single space after ":" with a single-line declaration   declaration-colon-space-after
 107:15   ⚠  Expected numeric font-weight notation                            font-weight-notation
 247:15   ⚠  Expected numeric font-weight notation                            font-weight-notation
 397:15   ⚠  Expected numeric font-weight notation                            font-weight-notation
 429:23   ⚠  Expected a leading zero                                          number-leading-zero
 433:11   ⚠  Expected a leading zero                                          number-leading-zero
 433:20   ⚠  Expected a leading zero                                          number-leading-zero
 510:9    ⚠  Unexpected named color "royalblue"                               color-named
 514:9    ⚠  Unexpected named color "purple"                                  color-named
 520:9    ⚠  Unexpected named color "midnightblue"                            color-named
 668:15   ⚠  Expected numeric font-weight notation                            font-weight-notation
 800:117  ⚠  Expected line length to be no more than 80 characters            max-line-length
 821:17   ⚠  Expected single space before "{"                                 block-opening-brace-space-before

Current result fro eslint

/_s/js/customizer.js
   9:1   error  Move the invocation into the parens that contain the function  wrap-iife
  15:5   error  There should be no spaces inside this paren                    space-in-parens
  16:4   error  There should be no spaces inside this paren                    space-in-parens
  20:5   error  There should be no spaces inside this paren                    space-in-parens
  21:4   error  There should be no spaces inside this paren                    space-in-parens
  27:46  error  There should be no spaces inside this paren                    space-in-parens
  30:7   error  There should be no spaces inside this paren                    space-in-parens
  32:46  error  There should be no spaces inside this paren                    space-in-parens
  35:7   error  There should be no spaces inside this paren                    space-in-parens
  36:48  error  There should be no spaces inside this paren                    space-in-parens
  38:7   error  There should be no spaces inside this paren                    space-in-parens
  40:5   error  There should be no spaces inside this paren                    space-in-parens
  41:4   error  There should be no spaces inside this paren                    space-in-parens
  42:3   error  There should be no spaces inside this paren                    space-in-parens

/_s/js/navigation.js
    7:1   error  Move the invocation into the parens that contain the function  wrap-iife
   85:37  error  Expected variable declaration to be on a new line              one-var-declaration-per-line
   90:57  error  There should be no spaces inside this paren                    space-in-parens
  106:3   error  There should be no spaces inside this paren                    space-in-parens

/_s/js/skip-link-focus-fix.js
   8:1  error  Move the invocation into the parens that contain the function  wrap-iife
  31:3  error  There should be no spaces inside this paren                    space-in-parens

Before this is merged I would like to see but only if it is decided that we want to do in this direction.

  • CSS Lint and ESLint run in travis
  • Styling issues fixed

@m-e-h
Copy link
Author

m-e-h commented Jul 6, 2017

A couple of the current lint errors that would need manually fixed are questionable whether or not we can and/or should even "fix" them.

  1. 6:277 ⚠ Expected line length to be no more than 80 characters
    This is the theme Description. Can't really be broken into multiple lines that I'm aware of.
  2. Several occurrences of Expected numeric font-weight notation This is for font-weight: bold. Should be fairly safe to change bold to 600, but in rare cases, this may not work correctly with a themes chosen font. Plus, the first occurrence is from Normalize. Probably want to leave that one be.
  3. Unexpected named color "royalblue" This is for the link styles. Any reason not to use the hex equivalent?

The alternatives to fixing these would be to:
a) add an exceptions to the Styleint config.

  "stylelint": {
    "extends": "stylelint-config-wordpress",
    "font-weight-notation": "numeric"|"named",
  },

or
b) Add an inline comment to the css

optgroup {
	font-weight: bold; /* stylelint-disable-line font-weight-notation */
}

I'm happy to submit a pull for correctly formatted SCSS files. Just wanted to get thoughts on the above fixes.

@grappler
Copy link
Collaborator

grappler commented Jul 6, 2017

  1. True, I justed to wanted to share all of the warning so that we could decide what to do. This may be something we add an exception for so that there is no warning.
  2. In the latest update of Normalize they have used bolder instead which stops the warning Update Normalize.css to v7.0.0 #1155
  3. This was added 6 years ago when _s first started and never updated a81dc6a

If we add an exception it should be added to the config file and not cluter the style.css. I don't think we should have an expectation for font-weight.

@m-e-h m-e-h force-pushed the add-packagejson branch 2 times, most recently from f13f515 to 0795d6c Compare July 7, 2017 20:13
@m-e-h m-e-h mentioned this pull request Jul 7, 2017
@m-e-h
Copy link
Author

m-e-h commented Jul 7, 2017

I've updated the devDependencies and scripts to use stylefmt for fixing lint errors. It's more mature than Stylelint --fix and supports fixing most rules we're interested in. https://github.com/morishitter/stylefmt#stylelint-rules-that-stylefmt-can-handle
This is the best way I can figure out to keep the formatting automatic.

I also discovered this https://github.com/stylelint/stylelint/tree/master/lib/rules/max-line-length#ignorepattern-regex which I've added to solve the Description: and the URL in comments max-line-length errors. It isn't working on the description though. Not sure I've got the regex right if anybody has any suggestions.

So after running npm run css-build with this updated SCSS branch #1168
I get this:

style.css
   6:277  ⚠  Expected line length to be no more than 80 characters   max-line-length     
 106:15   ⚠  Expected numeric font-weight notation                   font-weight-notation
 248:15   ⚠  Expected numeric font-weight notation                   font-weight-notation

Once normalize.css gets updated and we figure out the Description: regex, style.css generation should be pretty flawless.

package.json Outdated
"extends": "stylelint-config-wordpress",
"rules": {
"max-line-length": [ 80, {
"ignorePattern": ["/\b(Description:).*\n/", "/https?:\/\/[0-9,a-z]*.*/"]
Copy link
Contributor

Choose a reason for hiding this comment

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

It isn't working on the description though. Not sure I've got the regex right if anybody has any suggestions.

Just had a quick look at the regex and it looks fine to me, though the () around Description should not be needed.
Also, as we know that the Description should always be at the start of the line with - if anything - only whitespace before it, you could consider changing the \b to the slightly more specific: ^\s*.
All the same, the regex should work as is as well.

I'm wondering whether ignorePattern can actually take an array of patterns or whether the regexes should be merged into one ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This ['/(https?://[0-9,a-z]*.*)|(^description\\:.+)|(^tags\\:.+)/i'] should do the trick. Though we may not need it if we you the latest WordPress config.
WordPress-Coding-Standards/stylelint-config-wordpress#151

Copy link

Choose a reason for hiding this comment

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

Indeed, this shipped with stylelint-config-wordpress version 12.0.0, no need for it here.

@m-e-h
Copy link
Author

m-e-h commented Jul 7, 2017

@jrfnl just tried the Description without the https? pattern and got:

   6:277  ⚠  Expected line length to be no more than 80 characters   max-line-length     
  17:81   ⚠  Expected line length to be no more than 80 characters   max-line-length     
 108:15   ⚠  Expected numeric font-weight notation                   font-weight-notation
 250:15   ⚠  Expected numeric font-weight notation                   font-weight-notation
 349:88   ⚠  Expected line length to be no more than 80 characters   max-line-length

So it appears that the https pattern was working within the array.

Copy link

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

See inline comments on requested changes

package.json Outdated
"postcss-cli": "^4.1.0",
"stylefmt": "^6.0.0",
"stylelint": "^7.12.0",
"stylelint-config-wordpress": "^11.0.0"
Copy link

Choose a reason for hiding this comment

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

This should be updated to the latest 12.0.0 release.

package.json Outdated
"node-sass": "^4.5.3",
"postcss-cli": "^4.1.0",
"stylefmt": "^6.0.0",
"stylelint": "^7.12.0",
Copy link

Choose a reason for hiding this comment

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

This should be updated to the latest 8.0.0 release.

package.json Outdated
"autoprefixer": "^7.1.2",
"browserslist": "^2.1.5",
"eslint": "^4.1.1",
"eslint-config-wordpress": "^2.0.0",
Copy link

Choose a reason for hiding this comment

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

A new release of this is coming in the next couple of days, I'd suggest waiting until 3.0.0 is released as there are significant changes in this next release

package.json Outdated
"extends": "stylelint-config-wordpress",
"rules": {
"max-line-length": [ 80, {
"ignorePattern": ["/\b(Description:).*\n/", "/https?:\/\/[0-9,a-z]*.*/"]
Copy link

Choose a reason for hiding this comment

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

Indeed, this shipped with stylelint-config-wordpress version 12.0.0, no need for it here.

package.json Outdated
"css-prefix": "postcss style.css -u autoprefixer -o style.css --no-map",
"css-format": "stylefmt style.css",
"css-fix": "stylelint style.css --fix",
"scss-lint": "stylelint sass/**/*.scss",
Copy link

Choose a reason for hiding this comment

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

You need to specify that you are using the SCSS syntax here by adding --syntax scss to this command

It should also use the stylelint-config-wordpress/scss shared config for SCSS syntax per WPCS for SCSS

package.json Outdated
"> 1%",
"last 2 versions",
"IE 11"
]
Copy link

Choose a reason for hiding this comment

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

The browserlist matrix should match that of WordPress' https://make.wordpress.org/design/handbook/design-guide/browser-support/

You can see the above in code form [https://core.trac.wordpress.org/changeset/41062/trunk/Gruntfile.js here]

@ntwb
Copy link

ntwb commented Jul 24, 2017

This is for font-weight: bold. Should be fairly safe to change bold to 600, but in rare cases

bold should be replace with 700, not 600

@ntwb
Copy link

ntwb commented Jul 24, 2017

Lastly, per @davidakennedy comment here #1149 (comment)

  1. Automate compiling and have it still meet WordPress coding standards.

The two rules you were considering overriding max-line-length and font-weight-notation should not be as these are part of the WordPress CSS Coding Standards

@grappler
Copy link
Collaborator

grappler commented Jul 26, 2017

@m-e-h I started a PR #1187 to fix the coding standard issues in the JS files reported by PHPCS. I am thinking of adding support for ESLint at the same time once v0.3.0 for eslint-config-wordpress is released.

Would it be OK with you if this PR concentrated on the CSS and mine #1187 on the JS?

@m-e-h
Copy link
Author

m-e-h commented Jul 26, 2017

@grappler No problem! The JS stuff definitely shouldn't be held up by the CSS/Sass can of worms.

@ntwb Thanks for reviewing! I'll clean up this PR once the path for CSS clears up a bit.
Maybe you could give me your thoughts on using Stylefmt. It seems pretty necessary here for cleaning up the node-sass default formatting. Do you see Stylelints --fix feature catching up to it's rule coverage anytime soon?

@ntwb
Copy link

ntwb commented Jul 27, 2017

It seems pretty necessary here for cleaning up the node-sass default formatting.

Hmm,you'll need to look at some of the node-sass/libsass options I think

I'm guessing this is occurring because the compiled files are then being committed back to the repo, maybe these files should be excluded from the lint checks if they are only ever machine generated.

I'm not that familiar with what file is what in this repo so I'm not sure what the ideal solution here should be, with that said, see the follow up below for a future solution

Do you see Stylelints --fix feature catching up to it's rule coverage anytime soon?

Eventually, the plan is to basically deprecate stylefmt and have everything moved into stylelint

The v8 release we just released was ~8 months in the making and a few people are taking some time off so things are a little slow at the moment, but now that v8 is out we've a couple of focuses:

One is to add more -fix's to stylelint, the other is to create some pretty printers to support Prettier's CSS support, so with a combination of both more --fix's and Prettier coming to stylelint I'm sure a solution can be found that will work well for the above scenarios

@grappler grappler changed the title Add package.json for linting and compiling CSS: Add package.json for linting and compiling Aug 1, 2017
@grappler
Copy link
Collaborator

grappler commented Aug 1, 2017

@m-e-h As we have a decision in #1149 do you think you could update the PR with the changes @ntwb mentioned and only process the CSS files. Once those are made I can test it more easily.

@grappler
Copy link
Collaborator

@m-e-h Do you think you will have time to update the PR?

@m-e-h
Copy link
Author

m-e-h commented Sep 17, 2017

@grappler I've updated the PR to get us a little closer.

Do we want a command for linting the SCSS folder? I add the --syntax scss flag but I'm not sure how to extend both the stylelint-config-wordpress/scss and stylelint-config-wordpress shared configs.

Let me know what else I can change. Don't know that I'm completely clear on what all we want to do here.

@grappler
Copy link
Collaborator

Thanks @m-e-h I was playing around with it yesterday. This is how we could extend both configs.

    "extends": [
        "stylelint-config-wordpress",
        "stylelint-config-wordpress/scss"
    ],

I think the first step is to make all of the SCSS pass the linter.
Once it passes the linter we could add as a control point in travis so that pull requests fail if there is a styling issue.
Then we can look at the generating the style.css and getting that to pass the linter.
The last step would be to see that travis would automatically generate the style.css

All of this does not need to be done in this PR.

package.json Outdated
"css-prefix": "postcss style.css -u autoprefixer -o style.css --no-map",
"css-format": "stylefmt style.css",
"css-fix": "stylelint style.css --fix",
"scss-lint": "stylelint sass/**/*.scss --syntax scss"
Copy link

Choose a reason for hiding this comment

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

Change this to stylelint '**/*.scss' --syntax scss --config node_modules/stylelint-config-wordpress/scss.js

The glob needs to be quoted and loads the SCSS config from stylelint-config-wordpress

Copy link
Author

Choose a reason for hiding this comment

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

Updated. Thanks @ntwb !

@grappler
Copy link
Collaborator

@ntwb Thank you for your feedback. The main discussion of this was in #1149. You will be able to read up why we are moving to travis generating the style.css though it is not final as I see some other potential issues with auto commit in workflow.

Neither stylefmt or stylelint contain 100% fixable coverage as such they cannot be relied upon to fix every issue stylelint reports. (I wish stylelint did, but we're not there yet)

We have some solve this is smaller steps. I am not planning of making a PR for committing the style.css travis just yet. There are a few other things that we need to do before. I am happy to help create PR's for stylelint to get those remaining items patched.

@ntwb
Copy link

ntwb commented Sep 25, 2017

Thanks, I've read #1149 previously, so yeah, I guess we should walk before we run so to speak 😄

It would be great to get a hand on working on adding more fixable rules in stylelint @grappler 👍

@samikeijonen
Copy link
Contributor

What's the status of this PR?

@m-e-h
Copy link
Author

m-e-h commented Nov 9, 2017

@samikeijonen I'd like to get this thing moving.
Thing is, there are a lot of moving parts, issues, PRs involved.
Can we implement this before we merge the updated normalize.css PR? I think we probably can but for some reason I thought I was sorta waiting on that and a few other SASS/CSS related fixes.

It would be nice to have this in place before any total restructure like what's discussed here #1215

I'm game for whatever. Just let me know what we still need to do to get it merged.

@grappler
Copy link
Collaborator

grappler commented Nov 9, 2017

I would be Ok with this going in before anything else. I think this the most ready PR.

Though I did realize today that the generation of the layouts CSS is not supported but we may change a few things with the new structure.

We can improve the code style for the CSS/Sass and add it to travis later.

@samikeijonen
Copy link
Contributor

samikeijonen commented Nov 10, 2017

+1 for merging this first. Even if it's not perfect we need to start somewhere for merging all these moving parts. Then we can make this better.

added package-lock.json to .gitignore
added license
updated script naming convention
reordered scripts
updated deps
@m-e-h
Copy link
Author

m-e-h commented Nov 14, 2017

What other changes do we want to see? Any scripts I should remove?

Copy link
Collaborator

@grappler grappler left a comment

Choose a reason for hiding this comment

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

It looks good other than the one typo.

package.json Outdated
"compile:css": "node-sass sass/style.scss style.css --output-style expanded --indent-type tab --indent-width 1",
"prefix:css": "postcss style.css -u autoprefixer -o style.css --no-map",
"lint:css": "stylelint style.css",
"lint:css": "stylelint 'sass/**/*.scss' --syntax scss --config node_modules/stylelint-config-wordpress/scss.js",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be lint:scss

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah the latter one.

Copy link
Contributor

Choose a reason for hiding this comment

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

When I run npm install the line "lint:css": "stylelint style.css", was removed automatically. Probably because there can't be two same names.

Copy link
Author

Choose a reason for hiding this comment

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

@samikeijonen I've updated this. I had also forgotten to update the names when used within the build command. Should be working correctly now.

"stylelint-config-wordpress": "^12.0.0"
},
"stylelint": {
"defaultSeverity": "warning",
Copy link
Author

Choose a reason for hiding this comment

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

@grappler Not sure if this is wanted here?
I added it to get rid of errors printed in the terminal when linting didn't return 0 errors.

npm ERR! code ELIFECYCLE
npm ERR! errno 2
npm ERR! [email protected] fix:css: `stylelint style.css --fix`
npm ERR! Exit status 2
npm ERR! 
npm ERR! Failed at the [email protected] fix:css script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/marty/.npm/_logs/2017-11-15T15_48_26_137Z-debug.log
error Command failed with exit code 2.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am fine with leaving like this. We can look in the future to change the command to something like "stylelint '**/*.scss'; exit 0" if need be.

https://medium.com/@bjankord/how-to-lint-scss-with-stylelint-dc87809a9878

@samikeijonen
Copy link
Contributor

samikeijonen commented Nov 15, 2017

Thanks for typo fixes!

When I run npm run build:css I get weird empty lines before comments in style.css. Like this:

	z-index: 100000;

	/* Above WP toolbar. */

Not a big deal and we should probably use comment before the style in *.scss files.

@m-e-h
Copy link
Author

m-e-h commented Nov 15, 2017

Yep, the way single line comments are currently being used doesn't do well. I don't know how best to handle that. Using // would remove them all together from style.css. Not sure if that's OK or not..

@samikeijonen
Copy link
Contributor

samikeijonen commented Nov 15, 2017

We should leave the comments but I was thinking something like this in the original scss file.

/* Above WP toolbar. */
z-index: 100000;

Edit: Hmm with that it leaves one empty line above:

	width: auto;

	/* Above WP toolbar. */
	z-index: 100000;

@samikeijonen
Copy link
Contributor

Is it fix:css or format:css which are moving around the comments? They should just keep where they are.

@m-e-h
Copy link
Author

m-e-h commented Nov 15, 2017

This removes the empty line added above the comment:

  "stylelint": {
    "defaultSeverity": "warning",
    "extends": "stylelint-config-wordpress",
    "rules": {
        "comment-empty-line-before": "never"
    }
  }

but changes the rule entirely and we probably want it enforced elsewhere.

@m-e-h
Copy link
Author

m-e-h commented Nov 15, 2017

Just want to clarify what the plan is for moving forward with the package.json and the css build process.

Is the plan to merge this first and then use this build process to discuss and clean up the sass folder?

I don't think this file necessarily needs to compile or produce anything in this repo right away but the linting alone will be a good step towards keeping contributors on the same page as the sass gets cleaned up.

@grappler
Copy link
Collaborator

Is the plan to merge this first and then use this build process to discuss and clean up the sass folder?

That is how I see it. It is better to have smaller PR's then one big one.

@samikeijonen
Copy link
Contributor

We can safely merge this because at first state we're not building style.css with it.

I think there is roadmap somewhere in which order we should continue. If not, let's create one.

@samikeijonen
Copy link
Contributor

Any roadmap for merging this? I'll ping @davidakennedy just in case :)

@timurc
Copy link

timurc commented Feb 28, 2018

Hey! I was researching bare themes to start from, and this seems really one of the best to me. I would love to have a build process build in, this looks like a good start.

@Ismail-elkorchi
Copy link
Contributor

Ismail-elkorchi commented May 3, 2020

Thank you @m-e-h for the PR, it was a great inspiration for me.

I'm going to close this PR as linting and compiling tools were added in #1386, and #1391.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants