Thanks for your review Brad!
https://codereview.appspot.com/6844061/diff/1/Makefile File Makefile (right): https://codereview.appspot.com/6844061/diff/1/Makefile#newcode18 Makefile:18: PRODUCTION_FILES=build/juju-ui/assets/modules.js \ On 2012/11/19 18:18:58, bac wrote: > I'd be tempted to make a symbol for 'build/juju-ui/assets' since it is repeated > so often. Done. https://codereview.appspot.com/6844061/diff/1/Makefile#newcode89 Makefile:89: @cd build && python -m SimpleHTTPServer 8888 On 2012/11/19 18:18:58, bac wrote: > I'm unhappy that the magic port 8888 is found four times in our code and twice > in documentation. I wish it were possible to define it once, somewhere but am > at a loss for how to do it. I agree. I would like to create another card under the "Needs specification" group for it, so we can discuss this issue later. Is that ok? https://codereview.appspot.com/6844061/diff/1/Makefile#newcode94 Makefile:94: @rm -Rf build/ On 2012/11/19 18:18:58, bac wrote: > Yay, the clean target is much simplified. :O) 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. True. "install" does not mean "install". IMO "install" should be renamed to "generate-files", or something like that. wdyt? template.js should be in a separated rule, so we can determine when we should execute it, right? The same applies to the sprites. https://codereview.appspot.com/6844061/diff/1/app/index.html File app/index.html (right): https://codereview.appspot.com/6844061/diff/1/app/index.html#newcode14 app/index.html:14: <link rel="stylesheet" href="http://yui.yahooapis.com/combo?3.8.0pr1/build/slider-base/assets/skins/sam/slider-base.css";> On 2012/11/19 18:18:58, bac wrote: > Is this safe? The URL with an embedded version number looks fragile. True. I've investigated it a little bit further, and it works if I add the 'slider-base' requirement to the environment.js file. Tkx Brad! https://codereview.appspot.com/6844061/diff/1/app/index.html#newcode15 app/index.html:15: <link> On 2012/11/19 18:18:58, bac wrote: > What is the second <link> for? for nothing :) Removed tkx. https://codereview.appspot.com/6844061/diff/1/grunt.js File grunt.js (right): https://codereview.appspot.com/6844061/diff/1/grunt.js#newcode8 grunt.js:8: outputImage: 'stylesheets/sprite.png', On 2012/11/19 18:18:58, bac wrote: > I guess putting sprites in 'stylesheets' is a lie we can live with. I agree. 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
