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#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.
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 FILES=build/ juju-ui/ assets/ modules. js \ juju-ui/ assets' since it is
Makefile:18: PRODUCTION_
On 2012/11/19 18:18:58, bac wrote:
> I'd be tempted to make a symbol for 'build/
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 yui.yahooapis. com/combo? 3.8.0pr1/ build/slider- base/assets/ skins/sam/ slider- base.css">
app/index.html:14: <link rel="stylesheet"
href="http://
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 sprite. png',
grunt.js:8: outputImage: 'stylesheets/
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/