Implement ambiguous relation menu for adding rels

Implement ambiguous relation menu for adding rels

Displays a menu of choices when adding a relation between two services would result in ambiguity, otherwise simply adds a relation.  Also, a bit of minor test clean-up in charm config, due to floating point inequality.

https://code.launchpad.net/~makyo/juju-gui/ambiguous-relations/+merge/130616

(do not edit description out of merge proposal)

[matthew.scott, 2012-10-19 17:35:55]

Please take a look.

[thiago, 2012-10-19 18:09:28]

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#newcod...
app/views/environment.js:1449: var menu =
container.one('#ambiguous-relation-menu'),
If you are using ids you could simply use Y.one, right? Or you could use a class
instead.

https://codereview.appspot.com/6736051/diff/1/app/views/environment.js#newcod...
app/views/environment.js:1463: endpoints[m.id].forEach(function(endpoint) {
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?

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#newco...
lib/views/stylesheet.less:145: #service-menu, #ambiguous-relation-menu {
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

[matthew.scott, 2012-10-19 19:16:58]

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#newcod...
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#newcod...
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#newco...
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.

[matthew.scott, 2012-10-19 19:18:46]

Please take a look.

[benjamin.saller, 2012-10-19 19:19:04]

Thanks for this. A number of things, some of which will be good talks for next
week. For now I think updating the HTML generation is a good start and cards for
the ux cases mentioned below.

One, we've chosen different paths for self.state and self.set('state', ...). We
should agree to a pattern and stick with it. My feeling is that instance vars
were fine as we didn't depend on change events from Attribute manipulation. We
now have enough state on the env graph that I'd like to create ViewModels
around various operations and bind them to a Scene object. We're scribbling
scale/zoom/transform, click actions, current actions etc. If we don't contain
this better its only going to keep getting harder to develop. 


UX:

I'd title the menu, it may not be clear to users what is happening w/o some
'Add Relation...' menu entry as they may not be familiar with ambiguous
relations and their drag action just turned into something else.

Its now possible to have two relation between the same services. These will
draw in the same visual space and remove relation will need a way to control
the selection of which relation is being addressed. It would be a very special
case to detect if there is more than one relation sharing the same visual
space, I think a better solution is to develop a way that we can show both
relations exist which is not possible now. We need a card around addressing
this. The layout changes we plan to make must take this into account.
Additionally w/o the label disambiguation this case still won't be clear in
many cases.

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#newcod...
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?

Feeding the data to a template is the correct thing to do here. Single element 
DOM node creation is ok, but we avoid generating structures like this.

https://codereview.appspot.com/6736051/diff/1/app/views/environment.js#newcod...
app/views/environment.js:1488: menu.addClass('active');
setStyles can do these in one call. It does seem like we should be adding
support for scale/translate to Box and be using that for calculations like this.
Even though this is an HTML element we should still be able to wrap it in a box 

box.scaledPosition(scale, translate) -> {x, y, w, h} as with .pos today

https://codereview.appspot.com/6736051/diff/1/app/views/environment.js#newcod...
app/views/environment.js:1495: role: 'server' }],
I think you're right and we sort the endpoints, but we need to document were
taking advantage of this here, we shouldn't get client/server wrong.

https://codereview.appspot.com/6736051/diff/1/app/views/environment.js#newcod...
app/views/environment.js:1525: if (endpoints[0][0] === endpoints[1][0]) {
This is a better check, /me duhs.

[hazmat, 2012-10-19 21:48:58]

Looks pretty good. some minors below. could you a new card for the menu scroll
and positioning (ie. if too close to an edge, display on opposite side).

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

https://codereview.appspot.com/6736051/diff/3002/app/views/environment.js#new...
app/views/environment.js:1076: .ambiguousAddRelationTest(endpoint, self, rect);
i'd suggest renaming this method, just to avoid the test parlance outside of
unit testing. maybe... addRelationEndpointCheck

https://codereview.appspot.com/6736051/diff/3002/app/views/environment.js#new...
app/views/environment.js:1476: // Add a cancel item.
The cancel item is better at the end, preferably styled as a dialog cancel
button or with red/differentiating text.

https://codereview.appspot.com/6736051/diff/3002/app/views/environment.js#new...
app/views/environment.js:1477: menu.one('.cancel').on('click', function(evt) {
The menu should also cancel on click outside, the same way the invalid target
fadeout works.

https://codereview.appspot.com/6736051/diff/3002/app/views/environment.js#new...
app/views/environment.js:1483: var tr = view.zoom.translate(),
like the service menu this should also track roughly with the service container
on zoom/pan (the menu shouldn't resize though on zoom).

https://codereview.appspot.com/6736051/diff/3002/app/views/environment.js#new...
app/views/environment.js:1489: // Create a relation with the only available
endpoint.
i think this is better structured with the second clause upfront with a return
at the end of the block, and then the first block can become dedented.

https://codereview.appspot.com/6736051/diff/3002/test/test_environment_view.js
File test/test_environment_view.js (right):

https://codereview.appspot.com/6736051/diff/3002/test/test_environment_view.j...
test/test_environment_view.js:309: add_rel = container.one('#service-menu
.add-relation'),
indent looks a little off here.

[matthew.scott, 2012-10-20 18:14:16]

Cards created in FrontPage.

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

https://codereview.appspot.com/6736051/diff/3002/app/views/environment.js#new...
app/views/environment.js:1076: .ambiguousAddRelationTest(endpoint, self, rect);
On 2012/10/19 21:48:58, hazmat wrote:
> i'd suggest renaming this method, just to avoid the test parlance outside of
> unit testing. maybe... addRelationEndpointCheck

Done.

https://codereview.appspot.com/6736051/diff/3002/app/views/environment.js#new...
app/views/environment.js:1476: // Add a cancel item.
On 2012/10/19 21:48:58, hazmat wrote:
> The cancel item is better at the end, preferably styled as a dialog cancel
> button or with red/differentiating text.

Done.

https://codereview.appspot.com/6736051/diff/3002/app/views/environment.js#new...
app/views/environment.js:1477: menu.one('.cancel').on('click', function(evt) {
On 2012/10/19 21:48:58, hazmat wrote:
> The menu should also cancel on click outside, the same way the invalid target
> fadeout works.

Done.

https://codereview.appspot.com/6736051/diff/3002/app/views/environment.js#new...
app/views/environment.js:1483: var tr = view.zoom.translate(),
On 2012/10/19 21:48:58, hazmat wrote:
> like the service menu this should also track roughly with the service
container
> on zoom/pan (the menu shouldn't resize though on zoom).

Done.

https://codereview.appspot.com/6736051/diff/3002/app/views/environment.js#new...
app/views/environment.js:1489: // Create a relation with the only available
endpoint.
On 2012/10/19 21:48:58, hazmat wrote:
> i think this is better structured with the second clause upfront with a return
> at the end of the block, and then the first block can become dedented.

Done.

[matthew.scott, 2012-10-20 18:19:04]

*** Submitted:

Implement ambiguous relation menu for adding rels

Displays a menu of choices when adding a relation between two services would
result in ambiguity, otherwise simply adds a relation.  Also, a bit of minor
test clean-up in charm config, due to floating point inequality.

R=thiago, benjamin.saller, hazmat
CC=
https://codereview.appspot.com/6736051