yellow team mailing list archive

[Thiago Veronezi <thiago@xxxxxxxxxxxx>]

https://codereview.appspot.com/6821090/diff/15005/Makefile
File Makefile (right):

https://codereview.appspot.com/6821090/diff/15005/Makefile#newcode82
Makefile:82: @./bin/write-properties true
On 2012/11/09 13:57:38, gary.poster wrote:
> I really would like to see this (and the corresponding line in 87 and
the
> corresponding "write-properties" script) go.

> My preferred approach, which we talked about yesterday, would be to
make the
> server always have both the debug version (debug.html?) and the
production
> version (index.html) available, by having the server dynamically
change the js
> files that index.html points to.

> Alternatively, server.js would accept an argument to turn on debug
mode.  By
> default, it would run in prodcution mode.

Using the server approach for now.
Thanks for this idea. Really good.

https://codereview.appspot.com/6821090/diff/15005/app/modules-debug.js
File app/modules-debug.js (right):

https://codereview.appspot.com/6821090/diff/15005/app/modules-debug.js#newcode1
app/modules-debug.js:1: GlobalConfig = {
On 2012/11/09 13:57:38, gary.poster wrote:
> An idea (as in, do this if you agree): it would be nice to have a
comment at the
> top of this file saying that it is only used for the debug (developer)
views of
> the application, and pointing to the mechanism that is used (e.g., in
server.js)
> to serve this file rather than the other one.

> Maybe it's overkill.  I'd do it, but I'd be fine with accepting
disagreement.

Done.

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

https://codereview.appspot.com/6821090/diff/15005/bin/merge-files#newcode1
bin/merge-files:1: #!/usr/bin/env node
On 2012/11/09 13:57:38, gary.poster wrote:
> These changes look great to me.  Thanks.

Done.

https://codereview.appspot.com/6821090/diff/15005/bin/merge-files#newcode4
bin/merge-files:4: * We should aggregate and minimize the js sources in
order to improve the load
On 2012/11/09 13:57:38, gary.poster wrote:
> suggestion: delete "should".  (We are doing it. :-) )

Done.

https://codereview.appspot.com/6821090/diff/15005/bin/merge-files#newcode8
bin/merge-files:8: * to run from only static files and we want to be
able to run behaind a
On 2012/11/09 13:57:38, gary.poster wrote:
> "behind" (I realize these are probably my fault. sorry :-) )

Done.

https://codereview.appspot.com/6821090/diff/15005/bin/write-properties
File bin/write-properties (right):

https://codereview.appspot.com/6821090/diff/15005/bin/write-properties#newcode1
bin/write-properties:1: #!/usr/bin/env node
On 2012/11/09 13:57:38, gary.poster wrote:
> As I said, I'd like to delete.

Done.

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

https://codereview.appspot.com/6821090/diff/15005/lib/merge-files.js#newcode14
lib/merge-files.js:14: // end.
On 2012/11/09 13:57:38, gary.poster wrote:
> Another option (up to you): remove this comment, and do the export of
each
> function as you define it ("expose.minify = function(file) {...}" for
instance)
> or immediately after you define it.

Done.

https://codereview.appspot.com/6821090/diff/15005/lib/merge-files.js#newcode29
lib/merge-files.js:29: // or not)
On 2012/11/09 13:57:38, gary.poster wrote:
> Trivial: Please adjust the comment formatting.

My formatter formats comments... code and formatter fixed.

https://codereview.appspot.com/6821090/diff/15005/lib/merge-files.js#newcode44
lib/merge-files.js:44: // ignores
On 2012/11/09 13:57:38, gary.poster wrote:
> Trivial: Please adjust the comment formatting.

Done.

https://codereview.appspot.com/6821090/diff/15005/lib/merge-files.js#newcode75
lib/merge-files.js:75: // it is
On 2012/11/09 13:57:38, gary.poster wrote:
> Trivial: Please adjust the comment formatting.

Done.

https://codereview.appspot.com/6821090/diff/15005/lib/merge-files.js#newcode86
lib/merge-files.js:86: // (as
On 2012/11/09 13:57:38, gary.poster wrote:
> Trivial: Please adjust the comment formatting.

Done.

https://codereview.appspot.com/6821090/diff/15005/lib/merge-files.js#newcode128
lib/merge-files.js:128: // these
On 2012/11/09 13:57:38, gary.poster wrote:
> Trivial: Please adjust the comment formatting.

Done.

https://codereview.appspot.com/6821090/

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