Code review comment for lp://staging/~gary/juju-gui/syncMenu

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">
- <!-- Remove the query strings below after release 0.9.0. -->
- <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">
      <!-- Make sure the config is loaded before other JS for use in the page
           below.
@@ -363,9 +362,8 @@
        because we want the browser warning to execute before spending the
time
        to download an app the user might not be able to use anyway.
      -->
- <!-- Remove the query strings below after release 0.9.0. -->
- <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