yellow team mailing list archive

[Thiago Veronezi <thiago@xxxxxxxxxxxx>]

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.

https://codereview.appspot.com/6853044/

-- 
https://code.launchpad.net/~tveronezi/juju-gui/combinecss/+merge/134140
Your team Juju GUI Hackers is requested to review the proposed merge of lp:~tveronezi/juju-gui/combinecss into lp:juju-gui.