yellow team mailing list archive

[Benjamin Saller <benjamin.saller@xxxxxxxxxxxxx>]

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.