Minors and suggestions, but approved. I think this is a really good step forward on the topology stuff. Since a lot of this is fitting the current code into the new framework, much of the code is the same, though I did catch a few minors in there. I wonder if some additional comments in mega.js might help for the future branches direction different bits of code to different future branches? Additionally, there are some nested functions that may be torn up and moved about (there's a method that draws both services and relations, for example). These aren't necessarily for this branch, but it might be good to have some of this in writing, even if just in code reviews, to refer to later.
Thanks again for working on this; looking forward to better separation! 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#newcode189 app/assets/javascripts/d3-components.js:189: if (handler.context) { Could these two ifs be combined to help readability? 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"> <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> 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#newcode58 app/views/environment.js:58: // XXX: vomit Not a fan, I take it? 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#newcode538 app/views/topology/mega.js:538: // TODO:: figure out a clean way to update position I don't think this comment applies anymore, but will need to make sure. 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
