yellow team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

Thanks Thiago.  This is a nice improvement.  Once we have the CSS
changes, and we no longer have the slow initial combo CSS load from YUI,
this will be really nice to use.  I have some small suggestions for the
Makefile, in line with Brad's comments.


https://codereview.appspot.com/6844061/diff/1/.bzrignore
File .bzrignore (right):

https://codereview.appspot.com/6844061/diff/1/.bzrignore#newcode16
.bzrignore:16: build
Nice simplification and clean-up.

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

https://codereview.appspot.com/6844061/diff/1/Makefile#newcode100
Makefile:100:
On 2012/11/19 18:18:58, bac wrote:
> I'm concerned about separate build and install targets and having
template.js in
> a separate rule from the others.  Seems complicated and twisty.

Having "build" depend on "install" also seems odd, conceptually.

Does the "install" target bring any value?  That is, is there any use
case for running it alone?  If not, I suggest combining it and "build"
into the "build" target (and deleting the "install" target).  What do
you two think of that?

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

https://codereview.appspot.com/6844061/diff/1/lib/server.js#newcode57
lib/server.js:57: res.sendfile('build/juju-ui/assets/' + fileName);
Interesting.  I wanted the development server code to be even simpler
than this, but I can see why what you did is compelling.  I would be
tempted to experiment with converting the js files to symlinks in a
later branch and simplifying this code.  This is an improvement anyway,
though, and maybe my hunch won't work out.

https://codereview.appspot.com/6844061/

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