yellow team mailing list archive

[Brad Crittenden <bac@xxxxxxxxxxxxx>]

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.