Code review comment for lp:~makyo/juju-gui/ambiguous-relations

Thanks for the review, Thiago. See below for notes.

https://codereview.appspot.com/6736051/diff/1/app/views/environment.js
File app/views/environment.js (right):

https://codereview.appspot.com/6736051/diff/1/app/views/environment.js#newcode1449
app/views/environment.js:1449: var menu =
container.one('#ambiguous-relation-menu'),
On 2012/10/19 18:09:29, thiago wrote:
> If you are using ids you could simply use Y.one, right? Or you could
use a class
> instead.

I don't think that using a class would buy me anything here. The
ambiguous relation menu is unique, and referred to by a unique id.
Convention in the past has been to access nodes through container.

https://codereview.appspot.com/6736051/diff/1/app/views/environment.js#newcode1463
app/views/environment.js:1463:
endpoints[m.id].forEach(function(endpoint) {
On 2012/10/19 18:09:29, thiago wrote:
> You could use handlebars to build the list for you. For example:

> //remove old list, if any
> menu.all('ul').remove();
> .
> .
> .
> //create new <ul><li></li>...<ul> list
> var list = Templates.overviewRelationMenuList({
> endPoints: endpoints
> });

> list.all('li').on('click', ...

> menu.append(list);
> .
> .
> .

> This way you remove some html elements from this js file. wdyt?

Done. I initially couldn't figure out how I'd do it with Handlebars
given that the click action references each individual endpoint set, but
I did it with node data.

https://codereview.appspot.com/6736051/diff/1/lib/views/stylesheet.less
File lib/views/stylesheet.less (right):

https://codereview.appspot.com/6736051/diff/1/lib/views/stylesheet.less#newcode145
lib/views/stylesheet.less:145: #service-menu, #ambiguous-relation-menu {
On 2012/10/19 18:09:29, thiago wrote:
> You could remove the id and use both classes in your menu. Like...

> <div class="service-menu ambiguous-relation-menu">
> <div class="triangle">&nbsp;</div>
> <ul/>
> </div>
> </div>

> So you can remove this new css declaration and do something like...

> var menu = container.one('.ambiguous-relation-menu')

> ...in environment.js

Done. Both #service-menu and #ambiguous-relation-menu are unique and
must be referred to that way because of the way they're controlled in
the view. However, I have added a 'environment-menu' class to each, and
then used that as the selector here.

https://codereview.appspot.com/6736051/