Combine and minifiy CSS files.

Combine and minifiy CSS files.



https://code.launchpad.net/~benji/juju-gui/bug-1078978/+merge/137650

(do not edit description out of merge proposal)

[benji, 2012-12-03 18:27:54]

Please take a look.

[matthew.scott, 2012-12-03 21:57:20]

Request rereview (needs fixes).

I have one minor comment about the code, but was unable to complete a functional
test or a more in-depth look at the branch.  Running make build, server, or
debug gave me four of these errors: http://pastebin.ubuntu.com/1408941/ - this
happened both after first checking out the branch and also after make clean. 
When running the branch, I would get a 404 on all-static.css, all-yui.js, and
app.js; cache flushed and force-reloaded, same errors.

https://codereview.appspot.com/6865054/diff/1/lib/merge-files.js
File lib/merge-files.js (right):

https://codereview.appspot.com/6865054/diff/1/lib/merge-files.js#newcode27
lib/merge-files.js:27: var regex = /\(["']?((\.\/|\.\.\/)[^)]*)["']\)/g;
I think that this regex could use some more explanation or a more descriptive
variable name; it's a big one.

[bac, 2012-12-04 14:50:11]

Thanks for the changes Benji.  I've made some suggestions on making it more
understandable.  I have no need to re-review.

https://codereview.appspot.com/6865054/diff/1/Makefile
File Makefile (right):

https://codereview.appspot.com/6865054/diff/1/Makefile#newcode21
Makefile:21: $(BUILD_ASSETS_DIR)/combined-css/all-static.css
Thanks for making this sensible change.

https://codereview.appspot.com/6865054/diff/1/Makefile#newcode30
Makefile:30: cp -r --parents */assets "$(PWD)/$(BUILD_ASSETS_DIR)")
Why is $PWD required here but not at line 28?  Goose/gander, etc.

https://codereview.appspot.com/6865054/diff/1/Makefile#newcode86
Makefile:86: ln -sf `pwd`/node_modules/yui app/assets/javascripts/
Should we standardize on `pwd` vs $(PWD) ?  They are equivalent, no?

https://codereview.appspot.com/6865054/diff/1/bin/merge-files
File bin/merge-files (right):

https://codereview.appspot.com/6865054/diff/1/bin/merge-files#newcode9
bin/merge-files:9: * firewall without access to the internet.
Can we do this now?  Are we not getting anything from the Yahoo CDN?  Two
possible culprits are gallery-markdown and gallery-ellipsis.  I know they are
handled specially below but have you tried running while disconnected?

https://codereview.appspot.com/6865054/diff/1/bin/merge-files#newcode17
bin/merge-files:17: *     loader object. It means it wont even try to download
modules. We cannot
typo: won't

https://codereview.appspot.com/6865054/diff/1/bin/merge-files#newcode18
bin/merge-files:18: *     do it because the loader also manages the "use"
property which defines
"We cannot do it" -- what is "it"?  I cannot parse that sentence.

https://codereview.appspot.com/6865054/diff/1/grunt.js
File grunt.js (right):

https://codereview.appspot.com/6865054/diff/1/grunt.js#newcode1
grunt.js:1: 'use strict';
This file could you an introduction.  (Sorry, I know you've just inherited it.)

https://codereview.appspot.com/6865054/diff/1/lib/merge-files.js
File lib/merge-files.js (right):

https://codereview.appspot.com/6865054/diff/1/lib/merge-files.js#newcode27
lib/merge-files.js:27: var regex = /\(["']?((\.\/|\.\.\/)[^)]*)["']\)/g;
Oh, I thought that was a caterpillar emoticon.  Does it *do* something?

[benji, 2012-12-04 17:22:23]

Thanks for the good input guys.  I have fixed all the issues pointed out (plus a
couple more).  See my replies to individual comments for more detail.

https://codereview.appspot.com/6865054/diff/1/Makefile
File Makefile (right):

https://codereview.appspot.com/6865054/diff/1/Makefile#newcode30
Makefile:30: cp -r --parents */assets "$(PWD)/$(BUILD_ASSETS_DIR)")
On 2012/12/04 14:50:11, bac wrote:
> Why is $PWD required here but not at line 28?  Goose/gander, etc.

The $PWD is required here because we are running in a subshell (the parenthesis
wrapping the commands invoke a subshell) and inside that shell we are issuing a
"cd" command so the "cp -r --parents" will work the way we want (copying the
subdirectory structure).

https://codereview.appspot.com/6865054/diff/1/Makefile#newcode86
Makefile:86: ln -sf `pwd`/node_modules/yui app/assets/javascripts/
On 2012/12/04 14:50:11, bac wrote:
> Should we standardize on `pwd` vs $(PWD) ?  They are equivalent, no?

I was really just removing the unneccesary and silly "./" from the path, but
yes, we can just use one or the other.  I prefer $(PWD) because it does not
invoke a shell each time it is rendered so it does less work.  I have fixed
these.

https://codereview.appspot.com/6865054/diff/1/bin/merge-files
File bin/merge-files (right):

https://codereview.appspot.com/6865054/diff/1/bin/merge-files#newcode9
bin/merge-files:9: * firewall without access to the internet.
On 2012/12/04 14:50:11, bac wrote:
> Can we do this now?

If "this" is "use the YUI combo loader", then no.  The combo loader requires a
special combo server and we just want to use simple static HTTP servers.

> Are we not getting anything from the Yahoo CDN?  Two
> possible culprits are gallery-markdown and gallery-ellipsis.  I know they are
> handled specially below but have you tried running while disconnected?

I haven't tried running while disconnected, but the Chrome network panel clearly
shows them being loaded from an external site.

https://codereview.appspot.com/6865054/diff/1/bin/merge-files#newcode17
bin/merge-files:17: *     loader object. It means it wont even try to download
modules. We cannot
On 2012/12/04 14:50:11, bac wrote:
> typo: won't

Thanks, I didn't see that typo when I was editing this wall of text.

https://codereview.appspot.com/6865054/diff/1/bin/merge-files#newcode18
bin/merge-files:18: *     do it because the loader also manages the "use"
property which defines
On 2012/12/04 14:50:11, bac wrote:
> "We cannot do it" -- what is "it"?  I cannot parse that sentence.

I don't know.  I didn't write this text, I was just trying to fix it up as best
I could.

https://codereview.appspot.com/6865054/diff/1/grunt.js
File grunt.js (right):

https://codereview.appspot.com/6865054/diff/1/grunt.js#newcode1
grunt.js:1: 'use strict';
On 2012/12/04 14:50:11, bac wrote:
> This file could you an introduction.  (Sorry, I know you've just inherited
it.)

I've added a one-liner description of what the file does.  It's the best I have
without digging more deeply into it.

https://codereview.appspot.com/6865054/diff/1/lib/merge-files.js
File lib/merge-files.js (right):

https://codereview.appspot.com/6865054/diff/1/lib/merge-files.js#newcode27
lib/merge-files.js:27: var regex = /\(["']?((\.\/|\.\.\/)[^)]*)["']\)/g;
On 2012/12/03 21:57:20, matthew.scott wrote:
> I think that this regex could use some more explanation or a more descriptive
> variable name; it's a big one.

This function has been rendered unnecessary and has been removed.  You will just
have to spend the rest of your life wondering what that regex did.

[benji, 2012-12-04 17:34:12]

Please take a look.

[matthew.scott, 2012-12-04 19:18:19]

One minor, otherwise good to go.

Thanks for the fixes; definitely an overall win.

https://codereview.appspot.com/6865054/diff/8001/lib/merge-files.js
File lib/merge-files.js (right):

https://codereview.appspot.com/6865054/diff/8001/lib/merge-files.js#newcode26
lib/merge-files.js:26: function extractRelativePaths(s) {
Should be marked deprecated or removed (though it sounds like that's in the
works).

[benji, 2012-12-04 20:07:16]

Thanks for looking at this again.

https://codereview.appspot.com/6865054/diff/8001/lib/merge-files.js
File lib/merge-files.js (right):

https://codereview.appspot.com/6865054/diff/8001/lib/merge-files.js#newcode26
lib/merge-files.js:26: function extractRelativePaths(s) {
On 2012/12/04 19:18:19, matthew.scott wrote:
> Should be marked deprecated or removed (though it sounds like that's in the
> works).

Fixed.

[benji, 2012-12-04 20:09:33]

*** Submitted:

Combine and minifiy CSS files.

R=matthew.scott, bac
CC=
https://codereview.appspot.com/6865054