Merge lp:~jcsackett/juju-gui/browse-will-not-open-from-minimized into lp:juju-gui/experimental

[j.c.sackett]

Fixes the view nav for browse/build from minimized

-Adds viewmode controls to minimized
-Binds viewmode events into minimized
-Adds tests

https://codereview.appspot.com/13092044/

[j.c.sackett (jcsackett)]

Reviewers: mp+181151_code.launchpad.net,

Message:
Please take a look.

Description:
Fixes the view nav for browse/build from minimized

-Adds viewmode controls to minimized
-Binds viewmode events into minimized
-Adds tests

https://code.launchpad.net/~jcsackett/juju-gui/browse-will-not-open-from-minimized/+merge/181151

(do not edit description out of merge proposal)

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

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

[Richard Harding (rharding)]

LGTM with the test fixes to use done() vs the assert that might not get
hit based on the speed of the test run.

https://codereview.appspot.com/13092044/diff/1/test/test_browser_app.js
File test/test_browser_app.js (right):

https://codereview.appspot.com/13092044/diff/1/test/test_browser_app.js#newcode87
test/test_browser_app.js:87: it('can toggle back to oldviewmode via the
toggle', function() {
shouldn't these take done and then call it in the
view.on('viewNavigate')? Does this really assert anything or just pass
through? This is the same on all three tests.

https://codereview.appspot.com/13092044/

[Jeff Pihach (hatch)]

LGTM however I'm a little concerned about the duplication of code -
possibly create a follow-up to normalize this stuff?

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

https://codereview.appspot.com/13092044/diff/1/app/subapps/browser/views/minimized.js#newcode52
app/subapps/browser/views/minimized.js:52: Binds the viewmode control
events to navigation.
This code appears to be duplicated in other parts of the application -
can it not be split out into a mixin and then mixed into all classes
which need it?

https://codereview.appspot.com/13092044/

[Richard Harding (rharding)]

LGTM with the test fixes to use done() vs the assert that might not get
hit based on the speed of the test run.

https://codereview.appspot.com/13092044/

[j.c.sackett (jcsackett)]

On 2013/08/20 20:44:46, jeff.pihach wrote:
> LGTM however I'm a little concerned about the duplication of code -
possibly
> create a follow-up to normalize this stuff?

https://codereview.appspot.com/13092044/diff/1/app/subapps/browser/views/minimized.js
> File app/subapps/browser/views/minimized.js (right):

https://codereview.appspot.com/13092044/diff/1/app/subapps/browser/views/minimized.js#newcode52
> app/subapps/browser/views/minimized.js:52: Binds the viewmode control
events to
> navigation.
> This code appears to be duplicated in other parts of the application -
can it
> not be split out into a mixin and then mixed into all classes which
need it?

I agree it should be deduped, but the best way to do that is actually to
just have minimized also use MainView; since MainView sets up all the
search stuff, this should probably be done when we do the card "Make
search bar stay when sidebar is minimized". I'll add a comment to that
effect.

https://codereview.appspot.com/13092044/

[lp:~jcsackett/juju-gui/browse-will-not-open-from-minimized]

971. By j.c.sackett on 2013-08-21

Test changes from review.

972. By j.c.sackett on 2013-08-21

Comment about code duplication and future cleanup.

[Richard Harding (rharding)]

On 2013/08/21 12:57:03, j.c.sackett wrote:
> On 2013/08/20 20:44:46, jeff.pihach wrote:
> > LGTM however I'm a little concerned about the duplication of code -
possibly
> > create a follow-up to normalize this stuff?
> >
> >

https://codereview.appspot.com/13092044/diff/1/app/subapps/browser/views/minimized.js
> > File app/subapps/browser/views/minimized.js (right):
> >
> >

https://codereview.appspot.com/13092044/diff/1/app/subapps/browser/views/minimized.js#newcode52
> > app/subapps/browser/views/minimized.js:52: Binds the viewmode
control events
> to
> > navigation.
> > This code appears to be duplicated in other parts of the application
- can it
> > not be split out into a mixin and then mixed into all classes which
need it?

> I agree it should be deduped, but the best way to do that is actually
to just
> have minimized also use MainView; since MainView sets up all the
search stuff,
> this should probably be done when we do the card "Make search bar stay
when
> sidebar is minimized". I'll add a comment to that effect.

I was wondering if the viewmode-controls widget module could provide a
view extension that could be used to provide the setup/bind event
helpers for Views to use? I thought we had done this with another module
before.

https://codereview.appspot.com/13092044/

[lp:~jcsackett/juju-gui/browse-will-not-open-from-minimized]

973. By j.c.sackett on 2013-08-21

Merged trunk.

[j.c.sackett (jcsackett)]

*** Submitted:

Fixes the view nav for browse/build from minimized

-Adds viewmode controls to minimized
-Binds viewmode events into minimized
-Adds tests

R=rharding
CC=
https://codereview.appspot.com/13092044

https://codereview.appspot.com/13092044/

[j.c.sackett (jcsackett)]

*** Submitted:

Fixes the view nav for browse/build from minimized

-Adds viewmode controls to minimized
-Binds viewmode events into minimized
-Adds tests

R=rharding
CC=
https://codereview.appspot.com/13092044

https://codereview.appspot.com/13092044/

=== modified file 'app/subapps/browser/views/minimized.js'
--- app/subapps/browser/views/minimized.js	2013-08-02 15:34:18 +0000
+++ app/subapps/browser/views/minimized.js	2013-08-21 14:26:55 +0000
@@ -30,6 +30,11 @@
   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.
    *
@@ -37,7 +42,9 @@
    * @extends {Y.View}
    *
    */
-  ns.MinimizedView = Y.Base.create('browser-view-minimized', Y.View, [], {
+  ns.MinimizedView = Y.Base.create('browser-view-minimized', Y.View, [
+    Y.Event.EventTracker
+  ], {
     template: views.Templates.minimized,
 
     events: {
@@ -47,6 +54,62 @@
     },
 
     /**
+       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.
@@ -88,6 +151,13 @@
       var tpl = this.template(),
           tplNode = Y.Node.create(tpl);
       this.get('container').setHTML(tplNode);
+      // Make sure the controls starts out setting the correct active state
+      // based on the current viewmode for our View.
+      this.controls = new Y.juju.widgets.ViewmodeControls({
+        currentViewmode: this.get('oldViewMode')
+      });
+      this.controls.render();
+      this._bindViewmodeControls();
     }
 
   }, {
@@ -116,6 +186,7 @@
     'base',
     'juju-templates',
     'juju-views',
-    'view'
+    'view',
+    'viewmode-controls'
   ]
 });

=== modified file 'test/test_browser_app.js'
--- test/test_browser_app.js	2013-08-19 17:15:16 +0000
+++ test/test_browser_app.js	2013-08-21 14:26:55 +0000
@@ -33,6 +33,72 @@
   };
 
   (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;