Thanks for the changes Benji. I've made some suggestions on making it more understandable. I have no need to re-review.
https://codereview.appspot.com/6865054/diff/1/Makefile File Makefile (right): https://codereview.appspot.com/6865054/diff/1/Makefile#newcode21 Makefile:21: $(BUILD_ASSETS_DIR)/combined-css/all-static.css Thanks for making this sensible change. https://codereview.appspot.com/6865054/diff/1/Makefile#newcode30 Makefile:30: cp -r --parents */assets "$(PWD)/$(BUILD_ASSETS_DIR)") Why is $PWD required here but not at line 28? Goose/gander, etc. https://codereview.appspot.com/6865054/diff/1/Makefile#newcode86 Makefile:86: ln -sf `pwd`/node_modules/yui app/assets/javascripts/ Should we standardize on `pwd` vs $(PWD) ? They are equivalent, no? https://codereview.appspot.com/6865054/diff/1/bin/merge-files File bin/merge-files (right): https://codereview.appspot.com/6865054/diff/1/bin/merge-files#newcode9 bin/merge-files:9: * firewall without access to the internet. Can we do this now? Are we not getting anything from the Yahoo CDN? Two possible culprits are gallery-markdown and gallery-ellipsis. I know they are handled specially below but have you tried running while disconnected? https://codereview.appspot.com/6865054/diff/1/bin/merge-files#newcode17 bin/merge-files:17: * loader object. It means it wont even try to download modules. We cannot typo: won't https://codereview.appspot.com/6865054/diff/1/bin/merge-files#newcode18 bin/merge-files:18: * do it because the loader also manages the "use" property which defines "We cannot do it" -- what is "it"? I cannot parse that sentence. https://codereview.appspot.com/6865054/diff/1/grunt.js File grunt.js (right): https://codereview.appspot.com/6865054/diff/1/grunt.js#newcode1 grunt.js:1: 'use strict'; This file could you an introduction. (Sorry, I know you've just inherited it.) https://codereview.appspot.com/6865054/diff/1/lib/merge-files.js File lib/merge-files.js (right): https://codereview.appspot.com/6865054/diff/1/lib/merge-files.js#newcode27 lib/merge-files.js:27: var regex = /\(["']?((\.\/|\.\.\/)[^)]*)["']\)/g; Oh, I thought that was a caterpillar emoticon. Does it *do* something? https://codereview.appspot.com/6865054/ -- https://code.launchpad.net/~benji/juju-gui/bug-1078978/+merge/137650 Your team Juju GUI Hackers is requested to review the proposed merge of lp:~benji/juju-gui/bug-1078978 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
