| Thread Previous • Date Previous • Date Next • Thread Next |
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"> </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/
--
https://code.launchpad.net/~makyo/juju-gui/ambiguous-relations/+merge/130616
Your team Juju GUI Hackers is requested to review the proposed merge of lp:~makyo/juju-gui/ambiguous-relations into lp:juju-gui.
| Thread Previous • Date Previous • Date Next • Thread Next |
