Add functionality for removing subordinate rels

Add functionality for removing subordinate rels

Since subordinate relations were only visible on hover before, there was no means of removing one.  Now, if you click on the subordinate relation indicator on the side of the subordinate, the relations will persist, allowing you to delete them.  This is a toggle: click again to re-hide the relations.

https://code.launchpad.net/~makyo/juju-gui/remove-sub-rels/+merge/131725

(do not edit description out of merge proposal)

[matthew.scott, 2012-10-27 08:04:13]

Please take a look.

[gary.poster, 2012-11-06 14:14:39]

Hi Matt.  It has been awhile since this branch was proposed.  Sorry!

I merged trunk and got no conflicts.  However, tests failed, and adding
subordinate charms seemed broken.

I'm going to hold off on looking at this further until you've had a chance to
update the branch.  Sorry again for this taking so long.

Gary

[matthew.scott, 2012-11-07 20:59:18]

Please take a look.

[matthew.scott, 2012-11-07 21:03:11]

This branch is being proposed with the following already understood, from email
and IRC exchanges:

- There is a card already for the subordinate relations not having their events
bound properly when deployed (they're stuck at 0,0 and not draggable)
- Current branch of rapi-rollup fails on attempting to remove a subordinate
relation (this branch provides the UX for being able to do so)

[matthew.scott, 2012-11-07 21:03:30]

This branch is being proposed with the following already understood, from email
and IRC exchanges and experimentation:

- There is a card already for the subordinate relations not having their events
bound properly when deployed (they're stuck at 0,0 and not draggable)
- Current branch of rapi-rollup fails on attempting to remove a subordinate
relation (this branch provides the UX for being able to do so)

[gary.poster, 2012-11-07 21:57:18]

Hi Matt.  Looks good, thank you.

I have random UI/UX considerations for you to ponder or ignore.  Otherwise it
looks good to land.

- I wonder about making the active relation number italic in addition to blue,
fwiw.  If that's the desired behavior, cool.

- I guess it is fine that clicks off the relationship icon function normally and
keep the subordinate relationships drawn.  I kind of expected them to go away.
no big deal.

Thanks again,

Gary

[matthew.scott, 2012-11-08 21:55:59]

Please take a look.

[matthew.scott, 2012-11-08 21:57:39]

On 2012/11/07 21:57:18, gary.poster wrote:
> Hi Matt.  Looks good, thank you.
> 
> I have random UI/UX considerations for you to ponder or ignore.  Otherwise it
> looks good to land.
> 
> - I wonder about making the active relation number italic in addition to blue,
> fwiw.  If that's the desired behavior, cool.
> 
> - I guess it is fine that clicks off the relationship icon function normally
and
> keep the subordinate relationships drawn.  I kind of expected them to go away.
> no big deal.

Although I mentioned there was a branch for cancelling state, it was trivial to
pull this bit in in order to propose a nicer branch overall.

> 
> Thanks again,
> 
> Gary

[benji, 2012-11-13 16:41:48]

This branch looks good.

I had a couple of minor suggestions and a question.

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

https://codereview.appspot.com/6782063/diff/14001/app/views/environment.js#ne...
app/views/environment.js:1246: */
A @return entry would be nice. We have been using the recommended "@return
{undefined}" for methods that do not return anything.

https://codereview.appspot.com/6782063/diff/14001/app/views/environment.js#ne...
app/views/environment.js:1249:
this.addSVGClass(Y.one(subordinate).one('.sub-rel-count'), 'active');
If "subordinate" is already a node, why do we have to do the
"Y.one(subordinate)" dance?  Is it because it is a bare DOM node and we need a
YUI node?  If so, it might be good if the parameter comment included that fact.

https://codereview.appspot.com/6782063/diff/14001/app/views/environment.js#ne...
app/views/environment.js:1256: */
@return

https://codereview.appspot.com/6782063/diff/14001/app/views/environment.js#ne...
app/views/environment.js:1260:
this.removeSVGClass(Y.one('.sub-rel-count.active'), 'active');
Using a non-scoped Y.one() like this will be a problem if we ever have more than
one environment view on a page, but I suppose that is not something we should
worry about right now.

[matthew.scott, 2012-11-13 18:12:02]

Thanks!

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

https://codereview.appspot.com/6782063/diff/14001/app/views/environment.js#ne...
app/views/environment.js:1246: */
On 2012/11/13 16:41:48, benji wrote:
> A @return entry would be nice. We have been using the recommended "@return
> {undefined}" for methods that do not return anything.

Done.

https://codereview.appspot.com/6782063/diff/14001/app/views/environment.js#ne...
app/views/environment.js:1249:
this.addSVGClass(Y.one(subordinate).one('.sub-rel-count'), 'active');
On 2012/11/13 16:41:48, benji wrote:
> If "subordinate" is already a node, why do we have to do the
> "Y.one(subordinate)" dance?  Is it because it is a bare DOM node and we need a
> YUI node?  If so, it might be good if the parameter comment included that
fact.

Done.

https://codereview.appspot.com/6782063/diff/14001/app/views/environment.js#ne...
app/views/environment.js:1256: */
On 2012/11/13 16:41:48, benji wrote:
> @return

Done.

https://codereview.appspot.com/6782063/diff/14001/app/views/environment.js#ne...
app/views/environment.js:1260:
this.removeSVGClass(Y.one('.sub-rel-count.active'), 'active');
On 2012/11/13 16:41:48, benji wrote:
> Using a non-scoped Y.one() like this will be a problem if we ever have more
than
> one environment view on a page, but I suppose that is not something we should
> worry about right now.

Easy fix.  Done.

[matthew.scott, 2012-11-13 21:57:51]

*** Submitted:

Add functionality for removing subordinate rels

Since subordinate relations were only visible on hover before, there was no
means of removing one.  Now, if you click on the subordinate relation indicator
on the side of the subordinate, the relations will persist, allowing you to
delete them.  This is a toggle: click again to re-hide the relations.

R=gary.poster, benji
CC=
https://codereview.appspot.com/6782063