yellow team mailing list archive

[Thiago Veronezi <thiago@xxxxxxxxxxxx>]

Thanks Gary.


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

https://codereview.appspot.com/6821090/diff/7001/Makefile#newcode64
Makefile:64: combinejs: $(NODE_TARGETS)
On 2012/11/07 20:49:31, gary.poster wrote:
> As I mentioned on IRC, this has the same problem we talked about with
sprite
> generation: every time you run the target it will try to compress the
files.
> Since install depends on this, and just about everything depends on
install,
> this can affect most make targets and be pretty annoying.

> One approach to solving this is http://pastebin.ubuntu.com/1340722/

Done.

https://codereview.appspot.com/6821090/diff/7001/Makefile#newcode75
Makefile:75: @cp app/assets/combine/properties-dev.js
app/assets/javascripts/generated/properties.js
On 2012/11/07 17:05:18, benji wrote:
> This line is too long.

Done.

https://codereview.appspot.com/6821090/diff/7001/Makefile#newcode89
Makefile:89: @rm -Rf app/assets/javascripts/generated/
On 2012/11/07 20:49:31, gary.poster wrote:
> Good.

Done.

https://codereview.appspot.com/6821090/diff/7001/Makefile#newcode107
Makefile:107: combinejs
On 2012/11/07 20:49:31, gary.poster wrote:
> Good.

Done.

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js
File app/assets/combine/merge-files.js (right):

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js#newcode1
app/assets/combine/merge-files.js:1: 'use strict';
On 2012/11/07 20:49:31, gary.poster wrote:
> I don't think this file belongs in app/assets.  This is a build
component, and
> we never want to serve it as part of the application.  I suggest that
we put it
> in the top directory, or in bin.  I lean towards bin.

> This is as good a time as any to mention that I think this branch is a
great
> example of something that could have probably benefitted from a
> pre-implementation call (and maybe a prototyping).

This branch had a pre-implementation call. I moved this file here
because Ben asked me to do it.

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js#newcode3
app/assets/combine/merge-files.js:3: //
http://stackoverflow.com/questions/5348685/node-js-require-inheritance
On 2012/11/07 20:49:31, gary.poster wrote:
> Nice that you gave attribution.  I personally don't think it is
necessary in
> this case.  It was good to see as a reviewer, but it would have been
even better
> as part of your "cover letter" introduction to your branch.

This is not part of the global solution. This is here just to explain
why I use the "global" object.

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/merge-files.js#newcode6
app/assets/combine/merge-files.js:6: var Y = YUI(),
On 2012/11/07 20:49:31, gary.poster wrote:
> Why did you choose to do this rather than "YUI().use(..., function(Y)
{...});"?

> You could even remove line 4.  One approach:

> require('yui').YUI().use(...);

> another approach:

> var Y = require('yui').YUI(),

I did it because I dont require any internal yui code. I use it to get
the Y.Object, Y.Array and Y.Loader objects.

I cannot remove the line 4. It is used by the line 82. When node
executes the line 82, it loads one of our js files. These js files call
"YUI.add(..." and YUI is defined by "global.YUI".

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/properties-dev.js
File app/assets/combine/properties-dev.js (right):

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/properties-dev.js#newcode1
app/assets/combine/properties-dev.js:1: exports.debugMode = true;
On 2012/11/07 20:49:31, gary.poster wrote:
> So, AIUI, this is just a way to make the "make debug" target work.
Could we
> instead use a flag to merge-files.js and avoid messing with a file
like this?

Nice one tkx.

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/properties-prod.js
File app/assets/combine/properties-prod.js (right):

https://codereview.appspot.com/6821090/diff/7001/app/assets/combine/properties-prod.js#newcode1
app/assets/combine/properties-prod.js:1: exports.debugMode = false;
On 2012/11/07 20:49:31, gary.poster wrote:
> (same comment as with the dev file: I'd like to see this removed)

Done.

https://codereview.appspot.com/6821090/diff/7001/app/index.html
File app/index.html (right):

https://codereview.appspot.com/6821090/diff/7001/app/index.html#newcode72
app/index.html:72: <script src="/source/yui.js"></script>
On 2012/11/07 20:49:31, gary.poster wrote:
> On 2012/11/07 18:59:19, thiago wrote:
> > What about '/assets/yui.js'? Id this ok?

> Yeah, I think that's the kind of thing he means.

ok

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

https://codereview.appspot.com/6821090/diff/7001/app/modules-debug.js#newcode2
app/modules-debug.js:2: // Uncomment for debug versions of YUI.
On 2012/11/07 20:49:31, gary.poster wrote:
> Maybe we don't need this comment any more, since it is always in debug
mode?

Done.

https://codereview.appspot.com/6821090/diff/7001/app/modules-debug.js#newcode5
app/modules-debug.js:5: debug: false,
On 2012/11/07 20:49:31, gary.poster wrote:
> I suggest that we keep this one commented.  Otherwise, if you want to
leave this
> in place, I suggest that you revise the comment in line 4.

Comment revised.

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

https://codereview.appspot.com/6821090/diff/7001/app/modules.js#newcode3
app/modules.js:3: ignoreRegistered: true,
On 2012/11/07 20:49:31, gary.poster wrote:
> Maybe add a comment explaining what this does?

Done.

https://codereview.appspot.com/6821090/diff/7001/app/modules.js#newcode8
app/modules.js:8: 'juju-views': {
On 2012/11/07 20:49:31, gary.poster wrote:
> Why do we still need this?

We need it otherwise we need to define all the modules individually.
(and we still have the debug mode)

https://codereview.appspot.com/6821090/diff/7001/app/modules.js#newcode21
app/modules.js:21: 'juju-controllers': {
On 2012/11/07 20:49:31, gary.poster wrote:
> Same question

ditto.

https://codereview.appspot.com/6821090/diff/7001/lib/server.js
File lib/server.js (right):

https://codereview.appspot.com/6821090/diff/7001/lib/server.js#newcode50
lib/server.js:50: server.get('/source/all-third.js', function(req, res)
{
On 2012/11/07 18:59:19, thiago wrote:
> I want to hide the real path.
> I can change it to '/assets/all-third.js', would it be ok?

> On 2012/11/07 17:05:18, benji wrote:
> > Why should the path exposed via HTTP ("source") be different than
the path on
> > disk ("assets")?


Done.

https://codereview.appspot.com/6821090/diff/7001/lib/server.js#newcode56
lib/server.js:56:
res.sendfile('app/assets/javascripts/generated/all-app-debug.js');
On 2012/11/07 20:49:31, gary.poster wrote:
> Interesting.  so...we'll never have access to individual files?  Oh,
no, I see.
> Cool, nicely done.

> Something to consider is what a static version of the GUI would look
like,
> without server.js--and then how close we can get to that.  The closer
we can
> have our dev environment like our distributed environment, the better.
  Rather
> than having the server decide which .js file to serve, what about
having two
> different index.html files, one of which serves the debug and one of
which
> serves the normal version?

I had this idea. Ben didn't like it, and I agreed with him. :)

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.