Merge into trunk : syncMenu : Code : juju-gui

Status:	Merged
Merged at revision:	1053
Proposed branch:	lp://staging/~gary/juju-gui/syncMenu
Merge into:	lp://staging/juju-gui/experimental
Diff against target:	185 lines (+28/-48) 5 files modified app/index.html (+4/-6) app/views/environment.js (+5/-0) app/views/topology/service.js (+13/-25) test/test_environment_view.js (+5/-5) test/test_service_module.js (+1/-12)
To merge this branch:	bzr merge lp://staging/~gary/juju-gui/syncMenu
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Juju GUI Hackers		2013-09-17	Pending
Review via email: mp+186107@code.staging.launchpad.net

Description of the change

Sync relation menu with inspector open and close

While the relation menu is going away soonish, if the plans are to be believed, meanwhile it is what we have. This branch makes it so that when the inspector is first opened--after a ghost or when you click on a service--the relation menu appears. Similarly, when you close the inspector, relation menus close.

It is still possible to hide the relation menu while the inspector is open. If you begin to create a relation, the option will disappear; and if you click the canvas it will disappear. These seem like pretty good interactions for me, given the fundamental oddity (to me) of having the relation menu and the inspector separate entities, at least given the current design.

https://codereview.appspot.com/13583044/

Revision history for this message

Gary Poster (gary) wrote on 2013-09-17:

#

Download full text (9.1 KiB)

Reviewers: mp+186107_code.launchpad.net,

Message:
Please take a look.

Description:
Sync relation menu with inspector open and close

While the relation menu is going away soonish, if the plans are to be
believed, meanwhile it is what we have. This branch makes it so that
when the inspector is first opened--after a ghost or when you click on a
service--the relation menu appears. Similarly, when you close the
inspector, relation menus close.

It is still possible to hide the relation menu while the inspector is
open. If you begin to create a relation, the option will disappear; and
if you click the canvas it will disappear. These seem like pretty good
interactions for me, given the fundamental oddity (to me) of having the
relation menu and the inspector separate entities, at least given the
current design.

https://code.launchpad.net/~gary/juju-gui/syncMenu/+merge/186107

(do not edit description out of merge proposal)

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

Affected files (+30, -48 lines):
   A [revision details]
   M app/index.html
   M app/views/environment.js
   M app/views/topology/service.js
   M test/test_environment_view.js
   M test/test_service_module.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: app/index.html
=== modified file 'app/index.html'
--- app/index.html 2013-09-12 18:15:47 +0000
+++ app/index.html 2013-09-17 15:44:32 +0000
@@ -39,9 +39,8 @@
      <meta name="author" content="Juju team">
      <link
href='https://fonts.googleapis.com/css?family=Ubuntu+Mono:400,700|
Ubuntu:300,400,500,300italic,400italic,500italic' rel='stylesheet'
type='text/css'>
      <link rel="shortcut icon" href="/favicon.ico">
- 
- <link rel="stylesheet"
href="/juju-ui/assets/combined-css/all-static.css?reload=temporary">
- <link rel="stylesheet"
href="/juju-ui/assets/juju-gui.css?reload=temporary">
+ <link rel="stylesheet"
href="/juju-ui/assets/combined-css/all-static.css">
+ <link rel="stylesheet" href="/juju-ui/assets/juju-gui.css">
      <link rel="stylesheet" href="/juju-ui/assets/sprites.css">
      
- 
- <script src="/juju-ui/assets/all-yui.js?reload=temporary"></script>
- <script src="/juju-ui/assets/modules.js?reload=temporary"></script>
+ <script src="/juju-ui/assets/all-yui.js"></script>
+ <script src="/juju-ui/assets/modules.js"></script>
      <script>
        // Now that all of the above JS is loaded we can define the real
start
        // function which will be picked up by the setTimeout, and the ...

Reviewers: mp+186107_code.launchpad.net,

Message:
Please take a look.

Description:
Sync relation menu with inspector open and close

While the relation menu is going away soonish, if the plans are to be
believed, meanwhile it is what we have.  This branch makes it so that
when the inspector is first opened--after a ghost or when you click on a
service--the relation menu appears.  Similarly, when you close the
inspector, relation menus close.

It is still possible to hide the relation menu while the inspector is
open.  If you begin to create a relation, the option will disappear; and
if you click the canvas it will disappear.  These seem like pretty good
interactions for me, given the fundamental oddity (to me) of having the
relation menu and the inspector separate entities, at least given the
current design.

https://code.launchpad.net/~gary/juju-gui/syncMenu/+merge/186107

(do not edit description out of merge proposal)

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

Affected files (+30, -48 lines):
   A [revision details]
   M app/index.html
   M app/views/environment.js
   M app/views/topology/service.js
   M test/test_environment_view.js
   M test/test_service_module.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: gary.poster@canonical.com-20130917010610-wjknbz6bhfg3j8gt
+New revision: gary.poster@canonical.com-20130917170613-4blp40e0kdgt266u

Index: app/index.html
=== modified file 'app/index.html'
--- app/index.html	2013-09-12 18:15:47 +0000
+++ app/index.html	2013-09-17 15:44:32 +0000
@@ -39,9 +39,8 @@
      <meta name="author" content="Juju team">
      <link  
href='https://fonts.googleapis.com/css?family=Ubuntu+Mono:400,700| 
Ubuntu:300,400,500,300italic,400italic,500italic' rel='stylesheet'  
type='text/css'>
      <link rel="shortcut icon" href="/favicon.ico">
-    
-    <link rel="stylesheet"  
href="/juju-ui/assets/combined-css/all-static.css?reload=temporary">
-    <link rel="stylesheet"  
href="/juju-ui/assets/juju-gui.css?reload=temporary">
+    <link rel="stylesheet"  
href="/juju-ui/assets/combined-css/all-static.css">
+    <link rel="stylesheet" href="/juju-ui/assets/juju-gui.css">
      <link rel="stylesheet" href="/juju-ui/assets/sprites.css">
      
-    
-    <script src="/juju-ui/assets/all-yui.js?reload=temporary"></script>
-    <script src="/juju-ui/assets/modules.js?reload=temporary"></script>
+    <script src="/juju-ui/assets/all-yui.js"></script>
+    <script src="/juju-ui/assets/modules.js"></script>
      <script>
        // Now that all of the above JS is loaded we can define the real  
start
        // function which will be picked up by the setTimeout, and the app  
will

Index: test/test_environment_view.js
=== modified file 'test/test_environment_view.js'
--- test/test_environment_view.js	2013-09-13 13:47:51 +0000
+++ test/test_environment_view.js	2013-09-17 17:06:13 +0000
@@ -779,7 +779,7 @@
           var charm = {'id': service.charm,
                         loaded: false};
           db.charms.add(charm);
-         sm.toggleServiceMenu(service);
+         sm.showServiceMenu(service);

// Since the service's charm is not loaded the 'Build Relation'  
link
           // is disabled.
@@ -787,8 +787,8 @@
           charm = db.charms.getById(service.charm);
           charm.loaded = true;
           // Toggle the service menu twice to cause re-rendering.
-         sm.toggleServiceMenu(service);
-         sm.toggleServiceMenu(service);
+         sm.hideServiceMenu(service);
+         sm.showServiceMenu(service);
           // Now that the charm is loaded and the menu is re-rendered, the
           // Build Relation link is no longer disabled.
           assert.isFalse(add_rel.hasClass('disabled'));
@@ -815,7 +815,7 @@

// Toggle the service menu for the Add Relation button.
           var sm = view.topo.modules.ServiceModule;
-         sm.toggleServiceMenu(service);
+         sm.showServiceMenu(service);
           // Mock an event object so that d3.mouse does not throw a NPE.
           d3.event = {};
           add_rel.simulate('click');
@@ -878,7 +878,7 @@
           var module = view.topo.modules.RelationModule;
           var sm = view.topo.modules.ServiceModule;

-         sm.toggleServiceMenu(service);
+         sm.showServiceMenu(service);
           // Mock an event object so that d3.mouse does not throw a NPE.
           d3.event = {};
           add_rel.simulate('click');

Index: test/test_service_module.js
=== modified file 'test/test_service_module.js'
--- test/test_service_module.js	2013-09-03 09:41:49 +0000
+++ test/test_service_module.js	2013-09-17 17:06:13 +0000
@@ -147,17 +147,6 @@
      }
    });

-  it('should toggle the service menu',
-     function() {
-       var box = topo.service_boxes.haproxy;
-       var menu = viewContainer.one('#service-menu');
-       assert.isFalse(menu.hasClass('active'));
-       serviceModule.toggleServiceMenu(box);
-       assert(menu.hasClass('active'));
-       serviceModule.toggleServiceMenu(box);
-       assert.isFalse(menu.hasClass('active'));
-     });
-
    it('should show the service menu',
       function() {
         var box = topo.service_boxes.haproxy;
@@ -315,7 +304,7 @@
      var menu = view.get('container').one('#service-menu');
      view.topo.set('active_service', service);

-    serviceModule.toggleServiceMenu(box);
+    serviceModule.showServiceMenu(box);
      menu.one('.destroy-service').hasClass('disabled').should.equal(true);
    });

Index: app/views/environment.js
=== modified file 'app/views/environment.js'
--- app/views/environment.js	2013-09-13 20:11:50 +0000
+++ app/views/environment.js	2013-09-17 15:44:32 +0000
@@ -144,6 +144,11 @@
            // listen for the event and remove it from the list of open  
inspectors
            serviceInspector.viewletManager.after('destroy', function(e) {
              this.setInspector(e.currentTarget, true);
+            // We want the service menu to hide when the inspector does.
+            // For now, at least, with only one inspector, we can simply  
close
+            // all service menus.  We expect the service menus to go away
+            // soon-ish anyway in favor of a new approach.
+            this.topo.fire('hideServiceMenu');
            }, this);

// Restrict to a single inspector instance

Index: app/views/topology/service.js
=== modified file 'app/views/topology/service.js'
--- app/views/topology/service.js	2013-09-17 00:54:58 +0000
+++ app/views/topology/service.js	2013-09-17 17:06:13 +0000
@@ -107,12 +107,6 @@
          // Only update position if we're not already in a drag state (the
          // current drag supercedes any previous annotations).
          var fromGhost = d.model.get('placeFromGhostPosition');
-        if (fromGhost) {
-          // This flag has served its purpose, at initialization time on  
the
-          // canvas.  Remove it, so future changes will have the usual
-          // behavior.
-          d.model.set('placeFromGhostPosition', false);
-        }
          if (!d.inDrag) {
            var useTransitions = self.get('useTransitions') && !fromGhost;
            self.drag.call(this, d, self, {x: x, y: y}, useTransitions);
@@ -173,6 +167,18 @@
            'x': 64,
            'y': 47 * 0.8});

+    // Handle the last step of models that were made locally from ghosts.
+    node.filter(function(d) {
+      return d.model.get('placeFromGhostPosition');
+    }).each(function(d) {
+      // Show the service menu from the start.
+      self.showServiceMenu(d);
+      // This flag has served its purpose, at initialization time on the
+      // canvas.  Remove it, so future changes will have the usual
+      // behavior.
+      d.model.set('placeFromGhostPosition', false);
+    });
+
      // Landscape badge
      if (landscape) {
        node.each(function(d) {
@@ -454,8 +460,7 @@
      initializer: function(options) {
        ServiceModule.superclass.constructor.apply(this, arguments);
        // Set a default
-      this.set('currentServiceClickAction', 'toggleServiceMenu');
-
+      this.set('currentServiceClickAction', 'showServiceMenu');
      },

/**
@@ -1329,23 +1334,6 @@
      },

/**
-     * Show (if hidden) or hide (if shown) the service menu.
-     *
-     * @method toggleServiceMenu
-     * @param {object} box The presentation state for the service.
-     * @return {undefined} Side effects only.
-     */
-    toggleServiceMenu: function(box) {
-      var serviceMenu = this.get('container').one('#service-menu');
-
-      if (serviceMenu.hasClass('active') || !box) {
-        this.hideServiceMenu();
-      } else {
-        this.showServiceMenu(box);
-      }
-    },
-
-    /**
       * Show the service menu.
       *
       * @method showServiceMenu

Revision history for this message

Gary Poster (gary) wrote on 2013-09-17:

#

Comments

https://codereview.appspot.com/13583044/diff/1/app/index.html
File app/index.html (left):

https://codereview.appspot.com/13583044/diff/1/app/index.html#oldcode44
app/index.html:44: <link rel="stylesheet"
href="/juju-ui/assets/juju-gui.css?reload=temporary">
Flyby. We shouldn't need these any more, now that we've had the new
charm deployed for such a long time.

https://codereview.appspot.com/13583044/diff/1/app/views/topology/service.js
File app/views/topology/service.js (left):

https://codereview.appspot.com/13583044/diff/1/app/views/topology/service.js#oldcode114
app/views/topology/service.js:114: d.model.set('placeFromGhostPosition',
false);
Moved below.

https://codereview.appspot.com/13583044/diff/1/app/views/topology/service.js#oldcode1338
app/views/topology/service.js:1338: toggleServiceMenu: function(box) {
We don't use this any more.

https://codereview.appspot.com/13583044/diff/1/app/views/topology/service.js
File app/views/topology/service.js (right):

https://codereview.appspot.com/13583044/diff/1/app/views/topology/service.js#newcode463
app/views/topology/service.js:463: this.set('currentServiceClickAction',
'showServiceMenu');
Every time you click on a service, you will get the inspector and the
service menu.

https://codereview.appspot.com/13583044/

Revision history for this message

Madison Scott-Clary (makyo) wrote on 2013-09-17:

#

LGTM, QA okay

https://codereview.appspot.com/13583044/

Revision history for this message

Gary Poster (gary) wrote on 2013-09-17:

#

*** Submitted:

Sync relation menu with inspector open and close

While the relation menu is going away soonish, if the plans are to be
believed, meanwhile it is what we have. This branch makes it so that
when the inspector is first opened--after a ghost or when you click on a
service--the relation menu appears. Similarly, when you close the
inspector, relation menus close.

It is still possible to hide the relation menu while the inspector is
open. If you begin to create a relation, the option will disappear; and
if you click the canvas it will disappear. These seem like pretty good
interactions for me, given the fundamental oddity (to me) of having the
relation menu and the inspector separate entities, at least given the
current design.

R=matthew.scott
CC=
https://codereview.appspot.com/13583044

https://codereview.appspot.com/13583044/

juju-gui

Merge lp://staging/~gary/juju-gui/syncMenu into lp://staging/juju-gui/experimental

Commit message

Description of the change

Preview Diff

Subscribers