Merge lp:~jcsackett/juju-gui/view-controls-mixin into lp:juju-gui/experimental

[j.c.sackett]

Moves viewmode control binding to a mixin

* A new mixin has been created
* Nav functions like _goFullscreen have been moved to the mixin
* * toggleMinimized is just a placeholder, as fullscreen/sidebar need to do a
different thing than minimized
* The mixin doesn't create controls; it just has them passed in, since different
views setup the controls differently.

https://codereview.appspot.com/12935046/

[Richard Harding (rharding)]

Thanks for the cleanup.

I'd suggest keeping the extension in the widget code base. It's tied to the widget and needs to work with it. The Views are already importing that module in order to display the widget. The extension comes along for free. Then the widget tests can consume the tests for the extension as well to verify things work as they should work tied together. It's a pattern setup by the overlay-indicator widget.

Naming-wise I'd suggest something like ViewModeControlsView(Extension|Helper) or something.

#264 - you pass in controls, but use this.controls to bind the events.

The tests used in the current views should probably be pulled into testing this exntension directly. You could create an empty Y.View class, stick the extension on it, and make sure that the events fire with the right payload as it done currently in testing fullscreen/sidebar. Then they can not be duped in individual fullscreen/minimized/sidebar views.

[j.c.sackett (jcsackett)]

Reviewers: mp+181615_code.launchpad.net,

Message:
Please take a look.

Description:
Moves viewmode control binding to a mixin

* A new mixin has been created
* Nav functions like _goFullscreen have been moved to the mixin
* * toggleMinimized is just a placeholder, as fullscreen/sidebar need to
do a
different thing than minimized
* The mixin doesn't create controls; it just has them passed in, since
different
views setup the controls differently.
* Tests haven't been moved; seeing that the functionality works in the
views
seemes more valueable than setting up the necessary mocks to tests the
mixin
alone.

https://code.launchpad.net/~jcsackett/juju-gui/view-controls-mixin/+merge/181615

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M app/subapps/browser/views/minimized.js
   M app/subapps/browser/views/view.js
   M app/views/utils.js

[Richard Harding (rharding)]

Thanks for the cleanup.

I'd suggest keeping the extension in the widget code base. It's tied to
the widget and needs to work with it. The Views are already importing
that module in order to display the widget. The extension comes along
for free. Then the widget tests can consume the tests for the extension
as well to verify things work as they should work tied together. It's a
pattern setup by the overlay-indicator widget.

Naming-wise I'd suggest something like
ViewModeControlsView(Extension|Helper) or something.

The tests used in the current views should probably be pulled into
testing this exntension directly. You could create an empty Y.View
class, stick the extension on it, and make sure that the events fire
with the right payload as it done currently in testing
fullscreen/sidebar. Then they can not be duped in individual
fullscreen/minimized/sidebar views.

https://codereview.appspot.com/12935046/diff/1/app/views/utils.js
File app/views/utils.js (right):

https://codereview.appspot.com/12935046/diff/1/app/views/utils.js#newcode1484
app/views/utils.js:1484: _bindViewmodeControls: function(controls) {
you pass in controls, but continue to use this.controls for the events.

https://codereview.appspot.com/12935046/

[Madison Scott-Clary (makyo)]

LGTM with Rick's suggestions - good code and ideas; I'm good with Rick's
changes, too.

https://codereview.appspot.com/12935046/

[Jeff Pihach (hatch)]

LGTM with rick's comments about testing

https://codereview.appspot.com/12935046/diff/1/app/subapps/browser/views/view.js
File app/subapps/browser/views/view.js (right):

https://codereview.appspot.com/12935046/diff/1/app/subapps/browser/views/view.js#newcode44
app/subapps/browser/views/view.js:44:
Y.juju.views.utils.viewmodeControllingView
this can be shortened to just 'views' as it's defined above

https://codereview.appspot.com/12935046/diff/1/app/subapps/browser/views/view.js#newcode227
app/subapps/browser/views/view.js:227: _toggleMinimized: function(ev) {
I note that this is called by the extension would be helpful.

https://codereview.appspot.com/12935046/diff/1/app/views/utils.js
File app/views/utils.js (right):

https://codereview.appspot.com/12935046/diff/1/app/views/utils.js#newcode1471
app/views/utils.js:1471: * Constructor
This is technically incorrect, the constructor is the function on 1466

https://codereview.appspot.com/12935046/diff/1/app/views/utils.js#newcode1475
app/views/utils.js:1475: _initViewmodeControllingView: function() {
noop?

https://codereview.appspot.com/12935046/diff/1/app/views/utils.js#newcode1536
app/views/utils.js:1536: * @method _toggle_sidebar
_toggleMinimized

https://codereview.appspot.com/12935046/

[lp:~jcsackett/juju-gui/view-controls-mixin]

975. By j.c.sackett on 2013-08-22

Move viewmode controls extension to viewmode widget module.

976. By j.c.sackett on 2013-08-22

Test updates.

977. By j.c.sackett on 2013-08-22

Merged trunk.

978. By j.c.sackett on 2013-08-22

Shut up, lint.

[j.c.sackett (jcsackett)]

Please take a look.

https://codereview.appspot.com/12935046/

[Richard Harding (rharding)]

LGTM with comment on naming and a check that there's test cases for the
minimized method required by the two views fullscreen/sidebar.

https://codereview.appspot.com/12935046/diff/9001/app/subapps/browser/views/minimized.js
File app/subapps/browser/views/minimized.js (right):

https://codereview.appspot.com/12935046/diff/9001/app/subapps/browser/views/minimized.js#newcode40
app/subapps/browser/views/minimized.js:40:
Y.juju.widgets.ViewmodeControllingViewExtension
The widget is ViewmodeControls, I'd stick to that vs Controlling.

https://codereview.appspot.com/12935046/diff/9001/test/test_viewmode_controls_widget.js
File test/test_viewmode_controls_widget.js (right):

https://codereview.appspot.com/12935046/diff/9001/test/test_viewmode_controls_widget.js#newcode173
test/test_viewmode_controls_widget.js:173: view._toggleMinimized =
function() {
so we need individual tests on this then since it's not implemented in
the extension right?

https://codereview.appspot.com/12935046/

[j.c.sackett (jcsackett)]

On 2013/08/23 13:08:13, rharding wrote:

https://codereview.appspot.com/12935046/diff/9001/app/subapps/browser/views/minimized.js#newcode40
> app/subapps/browser/views/minimized.js:40:
> Y.juju.widgets.ViewmodeControllingViewExtension
> The widget is ViewmodeControls, I'd stick to that vs Controlling.

Made that switch.

https://codereview.appspot.com/12935046/diff/9001/test/test_viewmode_controls_widget.js#newcode173
> test/test_viewmode_controls_widget.js:173: view._toggleMinimized =
function() {
> so we need individual tests on this then since it's not implemented in
the
> extension right?

You're right; I hadn't intended to *completely* delete the minimzed
tests. Put one back in for the toggle to sidebar. Sidebar already has a
test for toggling to minimized.

https://codereview.appspot.com/12935046/

[lp:~jcsackett/juju-gui/view-controls-mixin]

979. By j.c.sackett on 2013-08-23

Comments from reviews.

980. By j.c.sackett on 2013-08-23

Test for minimized toggling to sidebar.

[j.c.sackett (jcsackett)]

*** Submitted:

Moves viewmode control binding to a mixin

* A new mixin has been created
* Nav functions like _goFullscreen have been moved to the mixin
* * toggleMinimized is just a placeholder, as fullscreen/sidebar need to
do a
different thing than minimized
* The mixin doesn't create controls; it just has them passed in, since
different
views setup the controls differently.

R=rharding, matthew.scott, jeff.pihach
CC=
https://codereview.appspot.com/12935046

https://codereview.appspot.com/12935046/

=== modified file 'app/subapps/browser/views/minimized.js'
--- app/subapps/browser/views/minimized.js	2013-08-21 13:00:08 +0000
+++ app/subapps/browser/views/minimized.js	2013-08-23 13:39:30 +0000
@@ -29,12 +29,6 @@
 YUI.add('subapp-browser-minimized', function(Y) {
   var ns = Y.namespace('juju.browser.views'),
       views = Y.namespace('juju.views');
-
-  // XXX jcsackett Aug 21 2013 The minimized view currently dupes a bunch of
-  // code from the Mainview. We can't just use mainview here b/c mainview
-  // expects search stuff to exist, which is only true for the sidebar and
-  // fullscreen views. When we add the search bar to always be present, we
-  // should remove this duplication.
   /**
    * The minimized state view.
    *
@@ -43,7 +37,7 @@
    *
    */
   ns.MinimizedView = Y.Base.create('browser-view-minimized', Y.View, [
-    Y.Event.EventTracker
+    Y.juju.widgets.ViewmodeControlsViewExtension
   ], {
     template: views.Templates.minimized,
 
@@ -54,71 +48,13 @@
     },
 
     /**
-       Binds the viewmode control events to navigation.
-
-       @method _bindViewmodeControls
-     */
-    _bindViewmodeControls: function() {
-      var container = this.get('container');
-      this.addEvent(
-          this.controls.on(
-              this.controls.EVT_TOGGLE_VIEWABLE, this._toggleViewState, this)
-      );
-
-      this.addEvent(
-          this.controls.on(
-              this.controls.EVT_FULLSCREEN, this._goFullscreen, this)
-      );
-      this.addEvent(
-          this.controls.on(
-              this.controls.EVT_SIDEBAR, this._goSidebar, this)
-      );
-    },
-
-    /**
-      Upon clicking the browser icon make sure we re-route to the
-      new form of the UX.
-
-      @method _goFullscreen
-      @param {Event} ev the click event handler on the button.
-
-     */
-    _goFullscreen: function(ev) {
-      ev.halt();
-      this.fire('viewNavigate', {
-        change: {
-          viewmode: 'fullscreen'
-        }
-      });
-    },
-
-    /**
-      Upon clicking the build icon make sure we re-route to the
-      new form of the UX.
-
-      @method _goSidebar
-      @param {Event} ev the click event handler on the button.
-
-     */
-    _goSidebar: function(ev) {
-      ev.halt();
-      this.fire('viewNavigate', {
-        change: {
-          viewmode: 'sidebar'
-        }
-      });
-    },
-
-    /**
-     * Toggle the visibility of the browser. Bound to nav controls in the
-     * view, however this will be expanded to be controlled from the new
-     * constant nav menu outside of the view once it's completed.
+     * Toggle the visibility of the browser.
      *
-     * @method _toggle_sidebar
+     * @method _toggleMinimized
      * @param {Event} ev event to trigger the toggle.
      *
      */
-    _toggleViewState: function(ev) {
+    _toggleMinimized: function(ev) {
       ev.halt();
 
       this.get('container').hide();
@@ -157,7 +93,7 @@
         currentViewmode: this.get('oldViewMode')
       });
       this.controls.render();
-      this._bindViewmodeControls();
+      this._bindViewmodeControls(this.controls);
     }
 
   }, {

=== modified file 'app/subapps/browser/views/view.js'
--- app/subapps/browser/views/view.js	2013-08-08 15:17:01 +0000
+++ app/subapps/browser/views/view.js	2013-08-23 13:39:30 +0000
@@ -40,7 +40,8 @@
    *
    */
   ns.MainView = Y.Base.create('browser-view-mainview', Y.View, [
-    Y.Event.EventTracker
+    Y.Event.EventTracker,
+    Y.juju.widgets.ViewmodeControlsViewExtension
   ], {
 
     /**
@@ -85,20 +86,6 @@
      */
     _bindSearchWidgetEvents: function() {
       var container = this.get('container');
-      this.addEvent(
-          this.controls.on(
-              this.controls.EVT_TOGGLE_VIEWABLE, this._toggleBrowser, this)
-      );
-
-      this.addEvent(
-          this.controls.on(
-              this.controls.EVT_FULLSCREEN, this._goFullscreen, this)
-      );
-      this.addEvent(
-          this.controls.on(
-              this.controls.EVT_SIDEBAR, this._goSidebar, this)
-      );
-
       if (this.search) {
         this.addEvent(
             this.search.on(
@@ -106,7 +93,6 @@
         );
       }
 
-
       if (this.search) {
         this.addEvent(
             this.search.on(
@@ -137,6 +123,7 @@
           }
         }, this);
       }
+      this._bindViewmodeControls(this.controls);
     },
 
     /**
@@ -233,11 +220,13 @@
        view, however this will be expanded to be controlled from the new
        constant nav menu outside of the view once it's completed.
 
-       @method _toggle_sidebar
+       This is called by the ViewmodeControlsViewExtension.
+
+       @method _toggleMinimized
        @param {Event} ev event to trigger the toggle.
 
      */
-    _toggleBrowser: function(ev) {
+    _toggleMinimized: function(ev) {
       ev.halt();
 
       this.fire('viewNavigate', {
@@ -248,44 +237,9 @@
     },
 
     /**
-      Upon clicking the browser icon make sure we re-route to the
-      new form of the UX.
-
-      @method _goFullscreen
-      @param {Event} ev the click event handler on the button.
-
-     */
-    _goFullscreen: function(ev) {
-      ev.halt();
-      this.fire('viewNavigate', {
-        change: {
-          viewmode: 'fullscreen'
-        }
-      });
-    },
-
-    /**
-      Upon clicking the build icon make sure we re-route to the
-      new form of the UX.
-
-      @method _goSidebar
-      @param {Event} ev the click event handler on the button.
-
-     */
-    _goSidebar: function(ev) {
-      ev.halt();
-      this.fire('viewNavigate', {
-        change: {
-          viewmode: 'sidebar'
-        }
-      });
-    },
-
-    /**
      * Destroy this view and clear from the dom world.
      *
      * @method destructor
-     *
      */
     destructor: function() {
       // Clean up any details view we might have hanging around.

=== modified file 'app/views/utils.js'
--- app/views/utils.js	2013-08-19 20:51:17 +0000
+++ app/views/utils.js	2013-08-23 13:39:30 +0000
@@ -1457,10 +1457,11 @@
     debugger;
     /*jshint debug:false */
   });
+
   /*
    * Extension for views to provide an apiFailure method.
    *
-   * @class apiFailure
+   * @class apiFailingView
    */
   utils.apiFailingView = function() {
     this._initAPIFailingView();

=== modified file 'app/widgets/viewmode-controls.js'
--- app/widgets/viewmode-controls.js	2013-07-15 13:17:42 +0000
+++ app/widgets/viewmode-controls.js	2013-08-23 13:39:30 +0000
@@ -189,6 +189,78 @@
     }
   });
 
+  /**
+   * Extension for views to provide viewmode controls.
+   *
+   * @class viewmodeControllingView
+   */
+  ns.ViewmodeControlsViewExtension = function() {};
+  ns.ViewmodeControlsViewExtension.prototype = {
+    /**
+     * Binds the viewmode controls on the page to the viewmode change events.
+     *
+     * @method _bindViewmodeControls
+     * @param {Y.Widget} controls The viewmode control widget.
+     */
+    _bindViewmodeControls: function(controls) {
+      this._fullscreen = controls.on(
+          controls.EVT_FULLSCREEN, this._goFullscreen, this);
+      this._sidebar = controls.on(
+          controls.EVT_SIDEBAR, this._goSidebar, this);
+      this._minimized = controls.on(
+          controls.EVT_TOGGLE_VIEWABLE, this._toggleMinimized, this);
+      this._destroy = this.on('destroy', function() {
+        this._fullscreen.detach();
+        this._sidebar.detach();
+        this._minimized.detach();
+        this._destroy.detach();
+      });
+    },
+
+    /**
+      Upon clicking the browser icon make sure we re-route to the
+      new form of the UX.
+
+      @method _goFullscreen
+      @param {Event} ev the click event handler on the button.
+
+     */
+    _goFullscreen: function(ev) {
+      ev.halt();
+      this.fire('viewNavigate', {
+        change: {
+          viewmode: 'fullscreen'
+        }
+      });
+    },
+
+    /**
+      Upon clicking the build icon make sure we re-route to the
+      new form of the UX.
+
+      @method _goSidebar
+      @param {Event} ev the click event handler on the button.
+
+     */
+    _goSidebar: function(ev) {
+      ev.halt();
+      this.fire('viewNavigate', {
+        change: {
+          viewmode: 'sidebar'
+        }
+      });
+    },
+
+    /**
+     * Place holder to toggle the minimized view; in minimized this should show
+     * sidebar, in sidebar this should show minimized.
+     * @method _toggleMinimized
+     * @param {Event} ev event to trigger the toggle.
+     *
+     */
+    _toggleMinimized: function(ev) {}
+  };
+
 }, '0.1.0', {
   requires: [
     'base',

=== modified file 'test/test_browser_app.js'
--- test/test_browser_app.js	2013-08-21 12:58:02 +0000
+++ test/test_browser_app.js	2013-08-23 13:39:30 +0000
@@ -33,72 +33,6 @@
   };
 
   (function() {
-    describe('browser minimized view', function() {
-      var browser, container, Minimized, view, views, Y;
-
-      before(function(done) {
-        Y = YUI(GlobalConfig).use(
-            'juju-views',
-            'juju-browser',
-            'juju-tests-utils',
-            'subapp-browser-minimized', function(Y) {
-              browser = Y.namespace('juju.browser');
-              views = Y.namespace('juju.browser.views');
-              Minimized = views.MinimizedView;
-              done();
-            });
-      });
-
-      beforeEach(function() {
-        container = Y.namespace('juju-tests.utils').makeContainer('container');
-        addBrowserContainer(Y, container);
-        // Mock out a dummy location for the Store used in view instances.
-        window.juju_config = {
-          charmworldURL: 'http://localhost'
-        };
-
-      });
-
-      afterEach(function() {
-        view.destroy();
-        Y.one('#subapp-browser').remove(true);
-        delete window.juju_config;
-        container.remove(true);
-      });
-
-      it('can route to fullscreen', function(done) {
-        view = new Minimized();
-        view.render();
-        view.on('viewNavigate', function(ev) {
-          assert.equal(ev.change.viewmode, 'fullscreen');
-          done();
-        });
-        view.controls._goFullscreen({halt: function() {} });
-      });
-
-      it('can route to sidebar via view controls', function(done) {
-        view = new Minimized();
-        view.render();
-        view.on('viewNavigate', function(ev) {
-          assert.equal(ev.change.viewmode, 'sidebar');
-          done();
-        });
-        view.controls._goSidebar({halt: function() {} });
-      });
-
-      it('can toggle back to oldviewmode via the toggle', function(done) {
-        view = new Minimized({oldViewmode: 'sidebar'});
-        view.render();
-        view.on('viewNavigate', function(ev) {
-          assert.equal(ev.change.viewmode, 'sidebar');
-          done();
-        });
-        view.controls._toggleViewable({halt: function() {} });
-      });
-    });
-  })();
-
-  (function() {
     describe('browser fullscreen view', function() {
       var browser, container, FullScreen, view, views, Y;
 
@@ -247,6 +181,54 @@
   })();
 
   (function() {
+    describe('browser minimzed view', function() {
+      var Y, browser, container, view, views, Minimized;
+
+      before(function(done) {
+        Y = YUI(GlobalConfig).use(
+            'juju-browser',
+            'juju-models',
+            'juju-views',
+            'juju-tests-utils',
+            'subapp-browser-minimized',
+            function(Y) {
+              browser = Y.namespace('juju.browser');
+              views = Y.namespace('juju.browser.views');
+              Minimized = views.MinimizedView;
+              done();
+            });
+      });
+
+      beforeEach(function() {
+        container = Y.namespace('juju-tests.utils').makeContainer('container');
+        addBrowserContainer(Y, container);
+        // Mock out a dummy location for the Store used in view instances.
+        window.juju_config = {
+          charmworldURL: 'http://localhost'
+        };
+      });
+
+      afterEach(function() {
+        view.destroy();
+        Y.one('#subapp-browser').remove(true);
+        delete window.juju_config;
+        container.remove(true);
+      });
+
+      it('toggles to sidebar', function(done) {
+        var container = Y.one('#subapp-browser');
+        view = new Minimized();
+        view.on('viewNavigate', function(ev) {
+          assert(ev.change.viewmode === 'sidebar');
+          done();
+        });
+        view.render(container);
+        view.controls._toggleViewable({halt: function() {}});
+      });
+    });
+  })();
+
+  (function() {
     describe('browser sidebar view', function() {
       var Y, browser, container, view, views, Sidebar;
 

=== modified file 'test/test_viewmode_controls_widget.js'
--- test/test_viewmode_controls_widget.js	2013-07-15 13:17:42 +0000
+++ test/test_viewmode_controls_widget.js	2013-08-23 13:39:30 +0000
@@ -106,3 +106,75 @@
     triggered.should.eql(true);
   });
 });
+
+describe('viewmode control extension', function() {
+  var Y, container, controls, ViewmodeControls, TestView, view;
+
+  before(function(done) {
+    Y = YUI(GlobalConfig).use(['viewmode-controls',
+                               'juju-tests-utils',
+                               'event-simulate',
+                               'node-event-simulate',
+                               'node'], function(Y) {
+      ViewmodeControls = Y.juju.widgets.ViewmodeControls;
+      TestView = Y.Base.create('testclass', Y.View, [
+        Y.juju.widgets.ViewmodeControlsViewExtension]);
+      done();
+    });
+  });
+
+  beforeEach(function() {
+    container = Y.namespace('juju-tests.utils').makeContainer('container');
+    Y.Node.create([
+      '<div id="content">',
+      '<div id="browser-nav">',
+      '<div class="sidebar"></div>',
+      '<div class="fullscreen"</div>',
+      '</div>'
+    ].join('')).appendTo(container);
+  });
+
+  afterEach(function() {
+    container.remove(true);
+    if (view) {
+      view.destroy();
+    }
+    if (controls) {
+      controls.destroy();
+    }
+  });
+
+  it('can route to fullscreen', function(done) {
+    view = new TestView();
+    controls = new ViewmodeControls();
+    view._bindViewmodeControls(controls);
+    view.on('viewNavigate', function(ev) {
+      assert.equal(ev.change.viewmode, 'fullscreen');
+      done();
+    });
+    controls._goFullscreen({halt: function() {} });
+  });
+
+  it('can route to sidebar via view controls', function(done) {
+    view = new TestView();
+    controls = new ViewmodeControls();
+    view._bindViewmodeControls(controls);
+    view.on('viewNavigate', function(ev) {
+      assert.equal(ev.change.viewmode, 'sidebar');
+      done();
+    });
+    controls._goSidebar({halt: function() {} });
+  });
+
+  it('can handle toggling the minimized state', function() {
+    var called = false;
+    view = new TestView();
+    controls = new ViewmodeControls();
+    view._toggleMinimized = function() {
+      called = true;
+    };
+    view._bindViewmodeControls(controls);
+    controls._toggleViewable({halt: function() {} });
+    assert.isTrue(called);
+  });
+});