yellow team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

Good, Thiago, thank you.  The slow CSS loading is the only downside for
putting this branch in staging, and if you can follow it quickly with
the CSS branch then I think it will be great.

+1!

Gary


https://codereview.appspot.com/6844061/diff/1/Makefile
File Makefile (right):

https://codereview.appspot.com/6844061/diff/1/Makefile#newcode89
Makefile:89: @cd build && python -m SimpleHTTPServer 8888
On 2012/11/19 18:50:59, thiago wrote:
> 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?

If you have any ideas on what we can do, +1.  Otherwise, eh, don't
bother IMO.

https://codereview.appspot.com/6844061/diff/1/lib/server.js
File lib/server.js (right):

https://codereview.appspot.com/6844061/diff/1/lib/server.js#newcode57
lib/server.js:57: res.sendfile('build/juju-ui/assets/' + fileName);
On 2012/11/19 20:23:19, thiago wrote:
> On 2012/11/19 19:00:33, gary.poster wrote:
> > Interesting.  I wanted the development server code to be even
simpler than
> this,
> > but I can see why what you did is compelling.  I would be tempted to
> experiment
> > with converting the js files to symlinks in a later branch and
simplifying
> this
> > code.  This is an improvement anyway, though, and maybe my hunch
won't work
> out.

> I needed to change the "test-server.js" file too. The tests were
broken due to
> the new notification stuff. I've tried to use the symbolic link
approach in
> order to avoid it, but with no success.

> I will use the "new branch" option to do it. Thanks.

Cool.  I'm not sure it is that worth it to mess with it, given that you
have already experimented with it.  That said, bug 1078910 will lead to
a natural opportunity to investigate.

https://codereview.appspot.com/6844061/diff/6002/Makefile
File Makefile (right):

https://codereview.appspot.com/6844061/diff/6002/Makefile#newcode16
Makefile:16: BUILD_ASSETS_DIR=build/juju-ui/assets
This ends up saving two characters per reference. :-)  I guess it is
still an advantage to prevent typos though.  Well...not even much there,
since bash doesn't have errors for unknown names.  I dunno, I'm
certainly fine with this, but it didn't end up helping much IMO.  Maybe
bac has a different suggestion for a name?

https://codereview.appspot.com/6844061/diff/6002/Makefile#newcode95
Makefile:95: build: appcache $(NODE_TARGETS) build/juju-ui/templates.js
yuidoc spritegen combinejs
I think "build" is the right name logically.  benji might not like the
fact that we are breaking backwards compatibility for the dev
experience, but since the "all" target is equivalent and still works, I
think it is fine.

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.