yellow team mailing list archive

[Thiago Veronezi <thiago@xxxxxxxxxxxx>]

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.