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. -- Mailing list: https://launchpad.net/~yellow Post to : [email protected] Unsubscribe : https://launchpad.net/~yellow More help : https://help.launchpad.net/ListHelp
