yellow team mailing list archive

[Brad Crittenden <bac@xxxxxxxxxxxxx>]

Thiago I think the work you've done here is good.  I'm a bit unclear how
much still needs to be done to finish off this refactoring of the build
structure.

Thanks for tackling it.  The branch looks good to land after you make
the changes suggested and weigh the other ones.


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 \
I'd be tempted to make a symbol for 'build/juju-ui/assets' since it is
repeated so often.

https://codereview.appspot.com/6844061/diff/1/Makefile#newcode89
Makefile:89: @cd build && python -m SimpleHTTPServer 8888
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.

https://codereview.appspot.com/6844061/diff/1/Makefile#newcode94
Makefile:94: @rm -Rf build/
Yay, the clean target is much simplified.

https://codereview.appspot.com/6844061/diff/1/Makefile#newcode100
Makefile:100:
I'm concerned about separate build and install targets and having
template.js in a separate rule from the others.  Seems complicated and
twisty.

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";>
Is this safe?  The URL with an embedded version number looks fragile.

https://codereview.appspot.com/6844061/diff/1/app/index.html#newcode15
app/index.html:15: <link>
What is the second <link> for?

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',
I guess putting sprites in 'stylesheets' is a lie we can live with.

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.