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. -- Mailing list: https://launchpad.net/~yellow Post to : [email protected] Unsubscribe : https://launchpad.net/~yellow More help : https://help.launchpad.net/ListHelp
