Merge lp:~makyo/juju-gui/add-rel-improvements into lp:juju-gui/experimental

[Madison Scott-Clary]

Add-relation improvements

UX improvements per Jovan: draglines should start with the long-click as an indicator that the user will be dragging a line; draglines should be on top of all services/lines, as they are currently the most important thing the user is working with; and draglines should be used when adding a relation from the service menu as well, showing that a relation will be created.

https://codereview.appspot.com/6786050/

[Madison Scott-Clary (makyo)]

Reviewers: mp+131542_code.launchpad.net,

Message:
Please take a look.

Description:
Add-relation improvements

UX improvements per Jovan: draglines should start with the long-click as
an indicator that the user will be dragging a line; draglines should be
on top of all services/lines, as they are currently the most important
thing the user is working with; and draglines should be used when adding
a relation from the service menu as well, showing that a relation will
be created.

https://code.launchpad.net/~makyo/juju-gui/add-rel-improvements/+merge/131542

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/6786050/

Affected files:
   A [revision details]
   M app/views/environment.js
   M test/test_environment_view.js

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: test/test_environment_view.js
=== modified file 'test/test_environment_view.js'
--- test/test_environment_view.js 2012-10-20 18:16:19 +0000
+++ test/test_environment_view.js 2012-10-26 07:54:00 +0000
@@ -339,6 +339,9 @@
           container.all('.selectable-service')
                 .size()
                 .should.equal(2);
+ container.all('.dragline')
+ .size()
+ .should.equal(1);
           service.next().simulate('click');
           container.all('.selectable-service').size()
              .should.equal(0);

Index: app/views/environment.js
=== modified file 'app/views/environment.js'
--- app/views/environment.js 2012-10-26 06:35:16 +0000
+++ app/views/environment.js 2012-10-26 07:48:26 +0000
@@ -33,6 +33,8 @@
                var box = this.get('active_service'),
                    service = this.serviceForBox(box),
                    context = this.get('active_context');
+ this.addRelationDragStart.call(this, box, context);
+ this.clickAddRelation = true;
                this.service_click_actions
                          .toggleControlPanel(box, this, context);
                this.service_click_actions
@@ -81,6 +83,12 @@
                if (!d.containsPoint(mouse_coords, self.zoom)) {
                  return;
                }
+
+ // Do not fire if we're on the same service.
+ if (d === self.get('addRelationStart_service')) {
+ return;
+ }
+
                self.set('potential_drop_point_service', d);
                self.set('potential_drop_point_rect', rect);
                self.addSVGClass(rect, 'hover');
@@ -162,6 +170,17 @@

container.all('.environment-menu.active').removeClass('active');
                self.service_click_actions.toggleControlPanel(null, self);
                self.cancelRelationBuild();
+ },
+ mousemove: function(d, self) {
+ // If we're clicking to add a relation, make sure a dragline
+ // follows the mouse cursor.
+ ...

[lp:~makyo/juju-gui/add-rel-improvements]

208. By Madison Scott-Clary on 2012-10-26

Add-relation improvements

209. By Madison Scott-Clary on 2012-10-26

Add-relation improvements

210. By Madison Scott-Clary on 2012-10-26

Add-relation improvements

[Madison Scott-Clary (makyo)]

Please take a look.

https://codereview.appspot.com/6786050/

[Gary Poster (gary)]

Hi Matt. This looks like it will be a step in a good direction. I just went for a cruise around the UX for a second before I looked at the code, and I saw a few things that you might want to address, or at least run past Jovan, before a more thorough review.

 - the two experiences of starting from long-click and starting from the service menu are different in ways that surprise me, and in at least one case seems like at error. In general I prefer the long-click behavior.
   * lines started from the service menu can go crazy when you hover over things that are not a target service. For example, try hovering over a relationship name. For me, the line then goes from the service start point to [0, 0]. I see other similar issues that I won't clarify now, but if you can't dupe I'll be happy to provide more details and screenshots. (This does not happen with long-click.)
   * lines started from the service menu cannot go over the service, unlike the change you made for long-click. I think they ought to be the same.

Also, unrelatedly but concerningly, I discovered bug 1072433. That looks pretty critical, but it is in the trunk. I put that in the kanban board.

I'm going to stop the review for now until I hear back from you on how you want to handle my UX observations.

Thanks,

Gary

[Madison Scott-Clary (makyo)]

On 10/28/2012 08:53 PM, Gary Poster wrote:
> Hi Matt. This looks like it will be a step in a good direction. I just went for a cruise around the UX for a second before I looked at the code, and I saw a few things that you might want to address, or at least run past Jovan, before a more thorough review.
>
> - the two experiences of starting from long-click and starting from the service menu are different in ways that surprise me, and in at least one case seems like at error. In general I prefer the long-click behavior.
> * lines started from the service menu can go crazy when you hover over things that are not a target service. For example, try hovering over a relationship name. For me, the line then goes from the service start point to [0, 0]. I see other similar issues that I won't clarify now, but if you can't dupe I'll be happy to provide more details and screenshots. (This does not happen with long-click.)
> * lines started from the service menu cannot go over the service, unlike the change you made for long-click. I think they ought to be the same.
>
> Also, unrelatedly but concerningly, I discovered bug 1072433. That looks pretty critical, but it is in the trunk. I put that in the kanban board.
>
> I'm going to stop the review for now until I hear back from you on how you want to handle my UX observations.
>
> Thanks,
>
> Gary
Thanks for the catch - looks like mousemove events aren't bubbling how I
thought they would, and I was working on a mostly empty environment.
I'll investigate all these points.

[lp:~makyo/juju-gui/add-rel-improvements]

211. By Madison Scott-Clary on 2012-10-30

Add-relation improvements

=== modified file 'app/views/environment.js'
--- app/views/environment.js	2012-10-29 10:01:29 +0000
+++ app/views/environment.js	2012-10-30 08:16:22 +0000
@@ -33,6 +33,8 @@
               var box = this.get('active_service'),
                   service = this.serviceForBox(box),
                   context = this.get('active_context');
+              this.addRelationDragStart.call(this, box, context);
+              this.clickAddRelation = true;
               this.service_click_actions
                         .toggleControlPanel(box, this, context);
               this.service_click_actions
@@ -81,6 +83,12 @@
               if (!d.containsPoint(mouse_coords, self.zoom)) {
                 return;
               }
+
+              // Do not fire if we're on the same service.
+              if (d === self.get('addRelationStart_service')) {
+                return;
+              }
+
               self.set('potential_drop_point_service', d);
               self.set('potential_drop_point_rect', rect);
               self.addSVGClass(rect, 'hover');
@@ -121,6 +129,13 @@
                 self.dragline.attr('class',
                     'relation pending-relation dragline dragging');
               }
+            },
+            mousemove: function(d, self) {
+              if (self.clickAddRelation) {
+                var container = self.get('container'),
+                    node = container.one('#canvas rect:first-child');
+                self.rectMousemove.call(node.getDOMNode(), d, self);
+              }
             }
           },
           '.sub-rel-block': {
@@ -152,7 +167,14 @@
 
           // Relation Related
           '.rel-label': {
-            click: 'relationClick'
+            click: 'relationClick',
+            mousemove: function(d, self) {
+              if (self.clickAddRelation) {
+                var container = self.get('container'),
+                    node = container.one('#canvas rect:first-child');
+                self.rectMousemove.call(node.getDOMNode(), d, self);
+              }
+            }
           },
 
           // Canvas related
@@ -162,7 +184,8 @@
               container.all('.environment-menu.active').removeClass('active');
               self.service_click_actions.toggleControlPanel(null, self);
               self.cancelRelationBuild();
-            }
+            },
+            mousemove: 'rectMousemove'
           }
         },
 
@@ -184,19 +207,6 @@
                 // we have the correct event in d3.event for d3.mouse().
                 d3.event = e;
 
-                // Flash an indicator around the center of the service block.
-                var center = d.getCenter();
-                self.vis.append('circle')
-                  .attr('cx', center[0])
-                  .attr('cy', center[1])
-                  .attr('r', 100)
-                  .attr('class', 'mouse-down-indicator')
-                  .transition()
-                  .duration(750)
-                  .ease('bounce')
-                  .attr('r', 0)
-                  .remove();
-
                 // Start the process of adding a relation
                 self.addRelationDragStart.call(self, d, this);
               }, [d, evt], false);
@@ -409,6 +419,18 @@
           self.removeRelationConfirm(d, this, self);
         },
 
+        rectMousemove: function(d, self) {
+          // If we're clicking to add a relation, make sure a dragline
+          // follows the mouse cursor.
+          if (self.clickAddRelation) {
+            var mouse = d3.mouse(this);
+            d3.event.x = mouse[0];
+            d3.event.y = mouse[1];
+            self.addRelationDrag
+              .call(self, self.get('addRelationStart_service'), this);
+          }
+        },
+
         /*
          * Sync view models with current db.models.
          */
@@ -1030,22 +1052,25 @@
 
         addRelationDragStart: function(d, context) {
           // Create a pending drag-line behind services.
-          var dragline = this.vis.insert('line', '.service')
+          var dragline = this.vis.append('line')
               .attr('class', 'relation pending-relation dragline dragging'),
               self = this;
 
-          // Start the line in the middle of the service.
-          var point = d.getCenter();
-          dragline.attr('x1', point[0])
-              .attr('y1', point[1])
-              .attr('x2', point[0])
-              .attr('y2', point[1]);
+          // Start the line between the cursor and the nearest connector
+          // point on the service.
+          var mouse = d3.mouse(Y.one('svg').getDOMNode());
+          self.cursorBox = views.BoundingBox();
+          self.cursorBox.pos = {x: mouse[0], y: mouse[1], w: 0, h: 0};
+          var point = self.cursorBox.getConnectorPair(d);
+          dragline.attr('x1', point[0][0])
+              .attr('y1', point[0][1])
+              .attr('x2', point[1][0])
+              .attr('y2', point[1][1]);
           self.dragline = dragline;
-          self.cursorBox = views.BoundingBox();
 
           // Start the add-relation process.
           self.service_click_actions
-          .addRelationStart(d, self, context);
+            .addRelationStart(d, self, context);
         },
 
         addRelationDrag: function(d, context) {
@@ -1142,6 +1167,7 @@
             this.dragline.remove();
             this.dragline = null;
           }
+          this.clickAddRelation = null;
           this.set('currentServiceClickAction', 'toggleControlPanel');
           this.show(this.vis.selectAll('.service'))
                   .classed('selectable-service', false);
@@ -1500,19 +1526,8 @@
               return a[0].name + a[1].name < b[0].name + b[1].name;
             });
 
-            // Create a pending line if it doesn't exist, as in the case of
-            // clicking to add relation.
-            if (!view.dragline) {
-              var dragline = view.vis.insert('line', '.service')
-                  .attr('class', 'relation pending-relation dragline'),
-                  points = m.getConnectorPair(
-                      view.get('addRelationStart_service'));
-              dragline.attr('x1', points[0][0])
-                .attr('y1', points[0][1])
-                .attr('x2', points[1][0])
-                .attr('y2', points[1][1]);
-              view.dragline = dragline;
-            }
+            // Stop rubberbanding on mousemove.
+            view.clickAddRelation = null;
 
             // Display menu with available endpoints.
             var menu = container.one('#ambiguous-relation-menu');
 
=== modified file 'test/test_environment_view.js'
--- test/test_environment_view.js	2012-10-25 09:52:10 +0000
+++ test/test_environment_view.js	2012-10-30 08:16:22 +0000
@@ -339,6 +339,9 @@
          container.all('.selectable-service')
                .size()
                .should.equal(2);
+         container.all('.dragline')
+               .size()
+               .should.equal(1);
          service.next().simulate('click');
          container.all('.selectable-service').size()
             .should.equal(0);