yellow team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

This is looking good, Thiago.  Thank you!

As we discussed, I'm OK with holding off on tests.  The last non-trivial
thing I'd like to see is removing the code that writes properties.  I
give two possible approaches below.

I'm happy to have a call if it helps, or leave it to you, as you prefer.

Thanks!

Gary


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
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.

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 = {
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.

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

https://codereview.appspot.com/6821090/diff/15005/app/modules.js#newcode4
app/modules.js:4: ignoreRegistered: true,
Is there a setting we can use to say "never download anything from the
internet, ever"?  If so, I suggest we use that both for this and the
debug modules file.

Is it bootstrap: false?  Or is this already handled somehow?

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
These changes look great to me.  Thanks.

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
suggestion: delete "should".  (We are doing it. :-) )

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
"behind" (I realize these are probably my fault. sorry :-) )

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
As I said, I'd like to delete.

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.
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.

https://codereview.appspot.com/6821090/diff/15005/lib/merge-files.js#newcode29
lib/merge-files.js:29: // or not)
Trivial: Please adjust the comment formatting.

https://codereview.appspot.com/6821090/diff/15005/lib/merge-files.js#newcode44
lib/merge-files.js:44: // ignores
Trivial: Please adjust the comment formatting.

https://codereview.appspot.com/6821090/diff/15005/lib/merge-files.js#newcode75
lib/merge-files.js:75: // it is
Trivial: Please adjust the comment formatting.

https://codereview.appspot.com/6821090/diff/15005/lib/merge-files.js#newcode86
lib/merge-files.js:86: // (as
Trivial: Please adjust the comment formatting.

https://codereview.appspot.com/6821090/diff/15005/lib/merge-files.js#newcode128
lib/merge-files.js:128: // these
Trivial: Please adjust the comment formatting.

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.