Thanks for these reviews. I went through and made changes and commented on most everything and will re-propose soon.
https://codereview.appspot.com/6867072/diff/1/app/assets/javascripts/d3-components.js File app/assets/javascripts/d3-components.js (right): https://codereview.appspot.com/6867072/diff/1/app/assets/javascripts/d3-components.js#newcode351 app/assets/javascripts/d3-components.js:351: * Called the first time render is invoked. See {render}. On 2012/12/10 18:00:02, benji wrote: > The standard yuidoc way, and the way all the existing yuidoc in the project is > written is with the @ directives at the end of the comment, not the beginning. > We have also been including @return and @param (@param isn't applicable here, > but I didn't want to repeat this for all the comments in the branch). I did the rearrangement, but I didn't add @return and @param to most of the methods, very few are callable in a normal sense, most are either chainable (which I've made an effort to label so) or are event handlers and return nothing. As methods are moved to individual modules the standard for both testing and docs will increase. https://codereview.appspot.com/6867072/diff/1/app/templates/overview.handlebars File app/templates/overview.handlebars (right): https://codereview.appspot.com/6867072/diff/1/app/templates/overview.handlebars#newcode1 app/templates/overview.handlebars:1: <div class="topology"> On 2012/12/08 00:02:42, matthew.scott wrote: > <bikeshedding>With this and other changes, part of me wants to rename the > overview.handlebars file to topology.handlebars or something similar, since the > concept and use of the topology is changing.</bikeshedding> I do agree, but I think we'll find there is a topology and then there is the main env view template (which includes menus and action items in its shell). I'll keep in mind that we should factor out the core of the template for use in other cases (like showing read only topos. https://codereview.appspot.com/6867072/diff/1/app/views/environment.js File app/views/environment.js (right): https://codereview.appspot.com/6867072/diff/1/app/views/environment.js#newcode19 app/views/environment.js:19: * @namespace juju.views On 2012/12/10 18:00:02, benji wrote: > The yuidoc specs prescribe not including the "base" namespace name ("juju") in > @namespace directives. I don't know why, but that also means I can't argue > against them. :) Done. https://codereview.appspot.com/6867072/diff/1/app/views/topology/mega.js File app/views/topology/mega.js (right): https://codereview.appspot.com/6867072/diff/1/app/views/topology/mega.js#newcode365 app/views/topology/mega.js:365: tree = this.tree, On 2012/12/10 18:00:02, benji wrote: > More weird indentation. Maybe you should dedent the entire file and let the > beautifier sort out the dead. Yeah, I re-ran it w/o the initial dedent and this is the kind of thing that happened, before that there were something like 800 errors coming out of the linter. https://codereview.appspot.com/6867072/diff/1/app/views/topology/panzoom.js File app/views/topology/panzoom.js (right): https://codereview.appspot.com/6867072/diff/1/app/views/topology/panzoom.js#newcode9 app/views/topology/panzoom.js:9: * @module topology-service On 2012/12/10 18:00:02, benji wrote: > Shouldn't the module name have something to do with panning and zooming? Yeah, these other modules are just templates really, they are dead code now, not loaded via addModule, I could just empty them out, but I'll correct things like this as the next branches will fill these out. https://codereview.appspot.com/6867072/diff/1/app/views/topology/service.js File app/views/topology/service.js (right): https://codereview.appspot.com/6867072/diff/1/app/views/topology/service.js#newcode18 app/views/topology/service.js:18: right: 0.084848}, On 2012/12/10 18:00:02, benji wrote: > I expect this is pre-existing code, but wow is that a very exact number. Yeah, it is. There are many levels of refactoring that should occur as this progresses, but we're on layout now, then structure, then I think details. https://codereview.appspot.com/6867072/diff/1/bin/lint-yuidoc File bin/lint-yuidoc (right): https://codereview.appspot.com/6867072/diff/1/bin/lint-yuidoc#newcode160 bin/lint-yuidoc:160: main() On 2012/12/10 18:00:02, benji wrote: > I don't think we want this new behavior. using the version from trunk now https://codereview.appspot.com/6867072/diff/1/test/test_application_notifications.js File test/test_application_notifications.js (right): https://codereview.appspot.com/6867072/diff/1/test/test_application_notifications.js#newcode198 test/test_application_notifications.js:198: it.skip('should show notification for "add_relation" and "remove_relation"' + On 2012/12/10 18:00:02, benji wrote: > Do we still need these skips? If so, we should have a bug and/or card to fix > them. For now, these have lines like views.environment.prototype.service_click_actions and depend pretty deeply on implementation details of the env view. I suspect in the future we can look at the notifications database and see if entries were added after actions are triggered, but we'll see, happy to add a card for this as the intent of the tests is valid. https://codereview.appspot.com/6867072/diff/1/test/test_application_notifications.js#newcode237 test/test_application_notifications.js:237: env = { On 2012/12/10 18:00:02, benji wrote: > I am not sure what the diff is trying to tell me for these lines. Any ideas? I think they were reindented https://codereview.appspot.com/6867072/diff/1/test/test_topology.js File test/test_topology.js (right): https://codereview.appspot.com/6867072/diff/1/test/test_topology.js#newcode51 test/test_topology.js:51: '</div>'); On 2012/12/10 18:00:02, benji wrote: > We are using chained node operations to build the test container now. The other > tests give good examples. I didn't find a better example. Is chaining better than a template in the test dir? https://codereview.appspot.com/6867072/ -- https://code.launchpad.net/~bcsaller/juju-gui/ultra-mega/+merge/138596 Your team Juju GUI Hackers is requested to review the proposed merge of lp:~bcsaller/juju-gui/ultra-mega 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
