yellow team mailing list archive

[Matthew Scott <matthew.scott@xxxxxxxxxxxxx>]

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.