Resource minification/aggregation css

Resource minification/aggregation css

* We should aggregate and minimize the css sources in order to improve the load speed of the application;
* We dont want to use the yui combo loader feature;
* The final product will provide three css files: one for the static dependencies, one for our generated templates and one our generated sprites.

https://code.launchpad.net/~tveronezi/juju-gui/combinecss/+merge/134140

(do not edit description out of merge proposal)

[thiago, 2012-11-13 15:53:06]

Please take a look.

[matthew.scott, 2012-11-13 17:08:24]

Some minors and one question for everyone, but looks good to me (and works well
after the new push).  Thanks.

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

https://codereview.appspot.com/6853044/diff/1/Makefile#newcode21
Makefile:21: app/assets/stylesheets/all-static.css
I think these are tabs (right angle-quotes in Rietveld).  Should probably be
spaces to be safe.

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

https://codereview.appspot.com/6853044/diff/1/bin/merge-files#newcode34
bin/merge-files:34: thirdPartyCss;
Minor, and totally open to debate (anyone else, feel free to chime in) - I
prefer thirdPartyCSS, personally, because it's an acronym and you pronounce each
of the letters.  Again, just a minor, though,

https://codereview.appspot.com/6853044/diff/1/lib/server.js
File lib/server.js (right):

https://codereview.appspot.com/6853044/diff/1/lib/server.js#newcode88
lib/server.js:88: // "./app/assets/stylesheets" directory.
I think this is a good way to do it, so long as the overhead isn't too great (I
expect not)

[frankban, 2012-11-13 17:27:42]

Looks good and the latest revision fixes the GlobalConfig problem. Thank you
Thiago.
Some comments follow.

https://codereview.appspot.com/6853044/diff/1/app/index.html
File app/index.html (right):

https://codereview.appspot.com/6853044/diff/1/app/index.html#newcode10
app/index.html:10: <link rel="stylesheet"
href="/assets/stylesheets/all-static.css">
Here the juju-ui prefix is missing.

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

https://codereview.appspot.com/6853044/diff/1/bin/merge-files#newcode34
bin/merge-files:34: thirdPartyCss;
On 2012/11/13 17:08:24, matthew.scott wrote:
> Minor, and totally open to debate (anyone else, feel free to chime in) - I
> prefer thirdPartyCSS, personally, because it's an acronym and you pronounce
each
> of the letters.  Again, just a minor, though,

FWIW, I prefer CSS too.

https://codereview.appspot.com/6853044/diff/1/lib/server.js
File lib/server.js (right):

https://codereview.appspot.com/6853044/diff/1/lib/server.js#newcode77
lib/server.js:77: server.get('/assets/stylesheets/:file', function(req, res) {
We discussed this on IRC: adding a 'juju-ui' prefix here and before (in
"/assets/all-third.js") allows us to easily integrate the gui in other projects.
As you said, express should still work after this change.

[thiago, 2012-11-13 18:19:16]

Thanks for your review, guys!

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

https://codereview.appspot.com/6853044/diff/1/Makefile#newcode21
Makefile:21: app/assets/stylesheets/all-static.css
On 2012/11/13 17:08:24, matthew.scott wrote:
> I think these are tabs (right angle-quotes in Rietveld).  Should probably be
> spaces to be safe.

Yeap. They are tabs.

I dont know if this is a Makefile rule, but in cases like this one above we
should use tabs instead of spaces.

https://codereview.appspot.com/6853044/diff/1/app/index.html
File app/index.html (right):

https://codereview.appspot.com/6853044/diff/1/app/index.html#newcode10
app/index.html:10: <link rel="stylesheet"
href="/assets/stylesheets/all-static.css">
On 2012/11/13 17:27:44, francesco.banconi wrote:
> Here the juju-ui prefix is missing.

Done.

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

https://codereview.appspot.com/6853044/diff/1/bin/merge-files#newcode34
bin/merge-files:34: thirdPartyCss;
On 2012/11/13 17:08:24, matthew.scott wrote:
> Minor, and totally open to debate (anyone else, feel free to chime in) - I
> prefer thirdPartyCSS, personally, because it's an acronym and you pronounce
each
> of the letters.  Again, just a minor, though,

Done.

https://codereview.appspot.com/6853044/diff/1/lib/server.js
File lib/server.js (right):

https://codereview.appspot.com/6853044/diff/1/lib/server.js#newcode77
lib/server.js:77: server.get('/assets/stylesheets/:file', function(req, res) {
On 2012/11/13 17:27:44, francesco.banconi wrote:
> We discussed this on IRC: adding a 'juju-ui' prefix here and before (in
> "/assets/all-third.js") allows us to easily integrate the gui in other
projects.
> As you said, express should still work after this change.

Done.

https://codereview.appspot.com/6853044/diff/1/lib/server.js#newcode88
lib/server.js:88: // "./app/assets/stylesheets" directory.
On 2012/11/13 17:08:24, matthew.scott wrote:
> I think this is a good way to do it, so long as the overhead isn't too great
(I
> expect not)

This is lightning fast.

[thiago, 2012-11-13 18:20:53]

Please take a look.

[bac, 2012-11-15 12:03:16]

Thanks Thiago.  This is another nice interim branch on the way to our simplified
build story.

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

https://codereview.appspot.com/6853044/diff/9001/bin/merge-files#newcode34
bin/merge-files:34: thirdPartyCSS;
Thanks for taking Matt's suggestion.  Acronyms in camelCase are always tricky
but I actually prefer this way.  It does get ugly if another word follows the
acronym, though, e.g. thirdPartyCSSFile.

https://codereview.appspot.com/6853044/diff/9001/lib/server.js
File lib/server.js (right):

https://codereview.appspot.com/6853044/diff/9001/lib/server.js#newcode15
lib/server.js:15: * It finds a file under the given path. The first match wins.
I'd rewrite as "Find a file" to be non-passive.

[thiago, 2012-11-15 13:09:21]

*** Submitted:

Resource minification/aggregation css

* We should aggregate and minimize the css sources in order to improve the load
speed of the application;
* We dont want to use the yui combo loader feature;
* The final product will provide three css files: one for the static
dependencies, one for our generated templates and one our generated sprites.

R=matthew.scott, francesco.banconi, bac
CC=
https://codereview.appspot.com/6853044