Updating build steps (CSS) #67

jandre3000 · 2018-02-20T14:20:52Z

Removing 'css/vendor' folder in favor of adding npm modules and
having them import automatically via postcss-import plugin.

Adding newer version of WikimediaUI Base & normalize.css via npm.

Fixing grunt watch task to ignore files in the 'css/build' folder

Adding npm tasks to call Grunt, for those who don't install Grunt globally.

Removing css/vendor folder in favor of adding NPM modules and having them import automatically via postcss-import plugin. Adding newer version of Wikimedia UI Base & normalize.css via NPM. Fixing grunt watch task to ignore files in the css/build folder Adding npm tasks to call grunt, for those who don't install grunt globally.

Volker-E · 2018-02-20T19:54:42Z

css/wmui-style-guide.dev.css

@@ -1,7 +1,12 @@
-@import 'vendor/wikimedia-ui-base.css';
+/* ::: Vendor Imports ::: */
+/* loaded from node_modules folders by postCSS-import plugin.


Volker-E · 2018-02-20T19:59:59Z

package.json

@@ -32,9 +32,12 @@
    "stylelint-config-wikimedia": "0.4.2"
  },
  "dependencies": {
-    "fontfaceobserver": "^2.0.9"
+    "fontfaceobserver": "^2.0.9",
+    "normalize.css": "^8.0.0",


As we're supporting old IE, we should pinpoint to an older version of normalize, which 7.0.0 on a quick look should be fine here. (And also adding a note about it)

Volker-E · 2018-02-20T20:01:04Z

package.json

-    "fontfaceobserver": "^2.0.9"
+    "fontfaceobserver": "^2.0.9",
+    "normalize.css": "^8.0.0",
+    "wikimedia-ui-base": "^0.10.0"
  },
  "scripts": {


So we've got those scripts for CI and npm stated above?

Oh shoot I forgot about CI. I will have to add an "npm test" script back in (that runs the default CI on GitHub).

In regards to adding npm run dev and npm run build, this might seem verbose but in my mind it actually makes things easier. I think npm install -> npm run dev -> npm run build -> npm run test is becoming more common across newer projects[1] (although now I think npm run start might be better than npm run dev ).

More importantly though, running grunt through npm means that you don't have to install grunt globally on your system. The grunt commands won't work unless you run npm install -g grunt after you run npm install. Running the gulp commands inside npm ensures that npm install actually installs all the dependencies you need[2]. Also, if you're working on many projects, you can run into issues with having different versions of grunt installed globally. This approach makes sure everyone who works on the project uses the same version of grunt.

1.https://github.com/facebook/create-react-app/blob/next/package.json
2.https://www.joezimjs.com/javascript/no-more-global-npm-packages/

Some asset paths were left unchanged after moving the css file to `css/build`. Removing the need for `grunt-contrib-cssmin` since PostCSS can integrate a minifier. This means we don't have to output an uncompressed css file in the `build` folder. Instead we can generate the minified file in one pass, which also gets rid of the cssmin grunt tasks. Also outputting a source map for the minified CSS file.

Volker-E · 2018-02-21T02:15:03Z

I think the outlined way makes sense here, would love to hear @Ladsgroup's and @prtksxna's opinions as well.

Ladsgroup · 2018-02-22T17:23:47Z

I think it's good idea to move towards using npm modules and making things more streamlined.

Volker-E · 2018-02-22T18:44:44Z

As stated in personal conversations with both, @j4n-co and @prtksxna the reason for the extra file step of committed wmui-style-guide.css has been an extra debugging insurance against something in PostCSS build step going wrong. PostCSS is mighty, but also sometimes easily misconfigured.
Therefore I'd prefer to stay with a middle step.

cscott · 2018-02-22T22:19:42Z

css/partials/fonts.css

@@ -1,16 +1,16 @@
 @font-face {
 	font-family: 'Charter';
-	src: url( '../fonts/Charter_regular.woff2' ) format( 'woff2' ),
-		 url( '../fonts/Charter_regular.woff' ) format( 'woff' );
+	src: url( '../../fonts/Charter_regular.woff2' ) format( 'woff2' ),


Yes, I just noticed that this was broken on the master branch. Thanks for fixing it...

Minified CSS gets compiles with source maps, unminified does not.

jandre3000 · 2018-02-23T09:26:51Z

@Volker-E 8b6f0c7 adds the compiled, unminified CSS file back in. Unfortunately, this does add some complexity to the grunt file. There are now two postcss 'subtasks' postcss:dev and postcss:min. Both take the source files and output dev/minified versions.

This is different than the previous cssmin task, which took the compiled file and minified it.

The reason for this difference is that it's valuable to have source maps with the minified version, and in order to properly create the source map, the source file has to be... the source. If we just minified the compiled version, we would have a source map pointing the compiled 'dev' css instead of the postcss.

I'm not too concerned that this will add a few milliseconds to the build.

Volker-E · 2018-02-23T17:59:04Z

Gruntfile.js

-				map: {
-					inline: false, // save all sourcemaps as separate files...
-					annotation: 'css/build/' // ...to the specified directory
+			// outputs unminified compiled CSS file into `build` dir


Comments preferred to start with uppercase letter

There's an eslint rule for that ( https://eslint.org/docs/rules/capitalized-comments ) if you wanted to enforce this.

Volker-E · 2018-02-23T17:59:37Z

Gruntfile.js

@@ -9,6 +9,31 @@ module.exports = function ( grunt ) {
 	grunt.loadNpmTasks( 'grunt-stylelint' );
 	grunt.loadNpmTasks( 'grunt-svgmin' );

+	// Postcss processors without minifier


”PostCSS“

Volker-E · 2018-02-23T19:45:38Z

Gruntfile.js

@@ -9,6 +9,31 @@ module.exports = function ( grunt ) {
 	grunt.loadNpmTasks( 'grunt-stylelint' );
 	grunt.loadNpmTasks( 'grunt-svgmin' );

+	// Postcss processors without minifier
+	var postcssProcessorsDev = [


postCssProcessorsDev is the MW way of writing it. Compare for example onResourceLoaderGetLessVars in mobile frontend.

Volker-E · 2018-02-23T19:45:56Z

Gruntfile.js

+		require( 'postcss-import' )( {
+			from: "css/wmui-style-guide.dev.css"
+		} ),
+		// require( 'postcss-cssnext' )(),


Why leave this in?

Volker-E · 2018-02-23T19:49:43Z

Gruntfile.js

-				files: {
-					'css/build/wmui-style-guide.min.css': 'css/wmui-style-guide.dev.css'
-				}
+			// outputs minified compiled CSS file +  src maps into `build` dir


Similar to above, instead of “outputs” “Output”, see also https://www.mediawiki.org/wiki/Manual:Coding_conventions/JavaScript#Documentation_comments
Also, extra whitespace after '+'

Volker-E

Comments inline

Volker-E

LGTM

- Removing 'css/vendor folder in favor of adding npm modules and having them import automatically via postcss-import plugin. - Adding newer version of WikimediaUI Base & normalize.css via npm. - Fixing 'grunt watch' task to ignore files in the 'css/build' folder - Adding npm tasks to call Grunt, for those who don't install Grunt globally. - Downgrading normalize.css to v7 & fixing typo - Fixing relative paths in CSS URLs & switching minifier to cssnano - Removing the need for `grunt-contrib-cssmin` since PostCSS can integrate a minifier. This means we don't have to output an uncompressed css file in the `build` folder. Instead we can generate the minified file in one pass, which also gets rid of the cssmin grunt tasks. - Also outputting a source map for the minified CSS file. - Adding seperate grunt tasks for compiling minified and unminifed CSS Minified CSS gets compiles with source maps, unminified does not.

jandre3000 requested review from prtksxna and Volker-E and removed request for prtksxna February 20, 2018 14:21

Volker-E reviewed Feb 20, 2018

View reviewed changes

Volker-E requested changes Feb 20, 2018

View reviewed changes

jandre3000 added 2 commits February 20, 2018 22:51

Downgrading normalize.css to v7 & fixing typo

f10acb7

Volker-E requested a review from Ladsgroup February 21, 2018 02:14

Removing package-lock.json and upgrading cssnano

08886b8

jandre3000 added 2 commits February 22, 2018 18:25

Removing package-lock.json from gitignore

4cd62bb

adding newline back to gitignore and missing spaces in gruntfile

727cbcf

cscott reviewed Feb 22, 2018

View reviewed changes

Adding seperate grunt tasks for compiling minified and unminifed CSS

8b6f0c7

Minified CSS gets compiles with source maps, unminified does not.

Volker-E reviewed Feb 23, 2018

View reviewed changes

Volker-E requested changes Feb 23, 2018

View reviewed changes

jandre3000 added 2 commits February 26, 2018 15:48

Grammar

9b3df59

Grammar

d4c9497

Volker-E approved these changes Feb 26, 2018

View reviewed changes

Volker-E merged commit 7a8ff8c into wikimedia:master Feb 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating build steps (CSS) #67

Updating build steps (CSS) #67

jandre3000 commented Feb 20, 2018 •

edited by Volker-E

Volker-E Feb 20, 2018

Volker-E Feb 20, 2018

Volker-E Feb 20, 2018

jandre3000 Feb 20, 2018

Volker-E commented Feb 21, 2018

Ladsgroup commented Feb 22, 2018

Volker-E commented Feb 22, 2018

cscott Feb 22, 2018

jandre3000 commented Feb 23, 2018

Volker-E Feb 23, 2018

cscott Feb 23, 2018

Volker-E Feb 23, 2018

Volker-E Feb 23, 2018

Volker-E Feb 23, 2018

Volker-E Feb 23, 2018

Volker-E left a comment •

edited

Volker-E left a comment

Updating build steps (CSS) #67

Updating build steps (CSS) #67

Conversation

jandre3000 commented Feb 20, 2018 • edited by Volker-E

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Volker-E commented Feb 21, 2018

Ladsgroup commented Feb 22, 2018

Volker-E commented Feb 22, 2018

Choose a reason for hiding this comment

jandre3000 commented Feb 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Volker-E left a comment • edited

Choose a reason for hiding this comment

Volker-E left a comment

Choose a reason for hiding this comment

jandre3000 commented Feb 20, 2018 •

edited by Volker-E

Volker-E left a comment •

edited