Merge lp:~jcsackett/juju-gui/abstract-default-viewmode into lp:juju-gui/experimental

[j.c.sackett]

Changes browser subapp default to be configurable

Rather than using sidebar, the browser subapp has a default viewmode as an attr.
Where sidebar was used before, this attr is instead used. You can qa it by
setting the default for the attr to fullscreen. For now at least, the default
for the attr remains "sidebar".

https://codereview.appspot.com/10870043/

[j.c.sackett (jcsackett)]

Reviewers: mp+172560_code.launchpad.net,

Message:
Please take a look.

Description:
Changes browser subapp default to be configurable

Rather than using sidebar, the browser subapp has a default viewmode as
an attr.
Where sidebar was used before, this attr is instead used. You can qa it
by
setting the default for the attr to fullscreen. For now at least, the
default
for the attr remains "sidebar".

https://code.launchpad.net/~jcsackett/juju-gui/abstract-default-viewmode/+merge/172560

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M app/subapps/browser/browser.js
   M test/test_browser_app.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_browser_app.js
=== modified file 'test/test_browser_app.js'
--- test/test_browser_app.js 2013-06-27 17:40:21 +0000
+++ test/test_browser_app.js 2013-07-02 13:25:43 +0000
@@ -242,29 +242,43 @@
        });
      });

- it('* route set sidebar by default', function() {
- app = new browser.Browser();
- // Stub out the sidebar so we don't render anything.
+ it('* route uses the default viewmode', function() {
+ app = new browser.Browser({default_viewmode: 'sidebar'});
        app.sidebar = function() {};
        var req = {
          path: '/'
        };

- app.routeSidebarDefault(req, null, next);
+ app.routeDefault(req, null, next);
        // The viewmode should be populated now to the default.
        assert.equal(req.params.viewmode, 'sidebar');
+
+ app = new browser.Browser({default_viewmode: 'fullscreen'});
+ app.fullscreen = function() {};
+ req = {
+ path: '/'
+ };
+ app.routeDefault(req, null, next);
+ assert.equal(req.params.viewmode, 'fullscreen');
      });

- it('prevents * route from doing more than sidebar by default',
function() {
- app = new browser.Browser();
+ it('prevents * route from doing more than the default', function() {
+ app = new browser.Browser({default_viewmode: 'sidebar'});
        var req = {
          path: '/sidebar'
        };

- app.routeSidebarDefault(req, null, next);
+ app.routeDefault(req, null, next);
        // The viewmode is ignored. This path isn't meant for this route
        // callable to deal with at all.
        assert.equal(req.params, undefined);
+
+ app = new browser.Browser({default_viewmode: 'fullscreen'});
+ req = {
+ path: '/fullscreen'
+ };
+ app.routeDefault(req, null, next);
+ assert.equal(req.params, undefined);
      });

      it('/charm/id routes to a sidebar view correcetly', function() {

Index: app/subapps/browser/browser.js
=== modified file 'app/subapps/browser/browser.js'
--- app/subapps/browser/browser.js 2013-06-27 18:47:57 +0000
+++ app/subapps/browser/browser.js 2013-07-01 19:04:02 +0000
@@ -81,8 +81,9 @@

        th...

[Richard Harding (rharding)]

LGTM with the change to the var name to defaultViewmode.

I'd prefer to see this along with the introduction of the config param
so that qa'ing it could be done with a change to the config file vs the
actual View's default ATTR value.

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

https://codereview.appspot.com/10870043/diff/1/app/subapps/browser/browser.js#newcode964
app/subapps/browser/browser.js:964: default_viewmode: {
camelCase please

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

https://codereview.appspot.com/10870043/diff/1/test/test_browser_app.js#newcode266
test/test_browser_app.js:266: app = new
browser.Browser({default_viewmode: 'sidebar'});
you don't need to add this here as it's picking up the default value.
Removing it verifies the default value is correct.

https://codereview.appspot.com/10870043/

[j.c.sackett (jcsackett)]

On 2013/07/02 13:55:48, rharding wrote:
> LGTM with the change to the var name to defaultViewmode.

> I'd prefer to see this along with the introduction of the config param
so that
> qa'ing it could be done with a change to the config file vs the actual
View's
> default ATTR value.

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

https://codereview.appspot.com/10870043/diff/1/app/subapps/browser/browser.js#newcode964
> app/subapps/browser/browser.js:964: default_viewmode: {
> camelCase please

Got it.

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

https://codereview.appspot.com/10870043/diff/1/test/test_browser_app.js#newcode266
> test/test_browser_app.js:266: app = new
browser.Browser({default_viewmode:
> 'sidebar'});
> you don't need to add this here as it's picking up the default value.
Removing
> it verifies the default value is correct.

I only passed it in so you can see the correlation between the set value
and the results of the test.

https://codereview.appspot.com/10870043/

[lp:~jcsackett/juju-gui/abstract-default-viewmode]

789. By j.c.sackett on 2013-07-02

camelCase

[j.c.sackett (jcsackett)]

Please take a look.

https://codereview.appspot.com/10870043/

[Jeff Pihach (hatch)]

LGTM'd thanks for this improvement!

As mentioned in IRC - next step being able to be set via the config.js
files.

https://codereview.appspot.com/10870043/diff/7001/test/test_browser_app.js
File test/test_browser_app.js (right):

https://codereview.appspot.com/10870043/diff/7001/test/test_browser_app.js#newcode245
test/test_browser_app.js:245: it('* route uses the default viewmode',
function() {
it('routes * by using the default viewmode' ...

PTDD (pedantic tdd) ;-)

https://codereview.appspot.com/10870043/

[j.c.sackett (jcsackett)]

On 2013/07/02 15:13:15, jeff.pihach wrote:
> LGTM'd thanks for this improvement!

> As mentioned in IRC - next step being able to be set via the config.js
files.

https://codereview.appspot.com/10870043/diff/7001/test/test_browser_app.js
> File test/test_browser_app.js (right):

https://codereview.appspot.com/10870043/diff/7001/test/test_browser_app.js#newcode245
> test/test_browser_app.js:245: it('* route uses the default viewmode',
function()
> {
> it('routes * by using the default viewmode' ...

> PTDD (pedantic tdd) ;-)

I think that's probably pedantic bdd. :-P I've fixed it.

https://codereview.appspot.com/10870043/

[lp:~jcsackett/juju-gui/abstract-default-viewmode]

790. By j.c.sackett on 2013-07-03

Merged colo:trunk

[j.c.sackett (jcsackett)]

*** Submitted:

Changes browser subapp default to be configurable

Rather than using sidebar, the browser subapp has a default viewmode as
an attr.
Where sidebar was used before, this attr is instead used. You can qa it
by
setting the default for the attr to fullscreen. For now at least, the
default
for the attr remains "sidebar".

R=rharding, jeff.pihach
CC=
https://codereview.appspot.com/10870043

https://codereview.appspot.com/10870043/

=== modified file 'app/subapps/browser/browser.js'
--- app/subapps/browser/browser.js	2013-06-27 18:47:57 +0000
+++ app/subapps/browser/browser.js	2013-07-03 16:37:36 +0000
@@ -81,8 +81,9 @@
 
       this._viewState = Y.merge(this._viewState, change);
 
-      if (this._viewState.viewmode !== 'sidebar' || this._viewState.search) {
-        // Sidebar is the default viewmode. There's no need to add it if we
+      if (this._viewState.viewmode !== this.get('defaultViewmode') ||
+          this._viewState.search) {
+        //There's no need to add the default view if we
         // don't need it. However it's currently required for search views to
         // match our current routes.
         urlParts.push(this._viewState.viewmode);
@@ -701,27 +702,28 @@
     },
 
     /**
-      When there's no charm or viewmode default to a sidebar view for all
+      When there's no charm or viewmode default to the default viewmode for all
       pages.
 
-      @method routeSidebarDefault
+      @method routeDefault
       @param {Request} req current request object.
       @param {Response} res current response object.
       @param {function} next callable for the next route in the chain.
 
      */
-    routeSidebarDefault: function(req, res, next) {
+    routeDefault: function(req, res, next) {
       // Check if there's any path. If there is, someone else will handle
       // routing it. Just carry on.
+      var viewmode = this.get('defaultViewmode');
       if (req.path.replace(/\//, '') !== '') {
         next();
         return;
       }
 
       // For the * request there will be no req.params. Update it forcing
-      // sidebar default viewmode.
+      // the default viewmode.
       req.params = {
-        viewmode: 'sidebar'
+        viewmode: viewmode
       };
 
       // Update the state for the rest of things to figure out what to do.
@@ -732,7 +734,7 @@
 
       // Don't bother routing if we're hidden.
       if (!this.hidden) {
-        this.sidebar(req, res, next);
+        this[viewmode](req, res, next);
       } else {
         // Let the next route go on.
         next();
@@ -755,6 +757,7 @@
      */
     routeDirectCharmId: function(req, res, next) {
       // If we don't have a valid store we can't do any work here.
+      var viewmode = this.get('defaultViewmode');
       if (!this._hasValidStore()) {
         return;
       }
@@ -775,7 +778,7 @@
         // We've got a valid id. Setup the params for our view state.
         req.params = {
           id: id,
-          viewmode: 'sidebar'
+          viewmode: viewmode
         };
       }
 
@@ -787,7 +790,7 @@
 
       // Don't bother routing if we're hidden.
       if (!this.hidden) {
-        this.sidebar(req, res, next);
+        this[viewmode](req, res, next);
       } else {
         // Let the next route go on.
         next();
@@ -809,7 +812,7 @@
       }
 
       if (!req.params.viewmode) {
-        req.params.viewmode = 'sidebar';
+        req.params.viewmode = this.get('defaultViewmode');
       }
 
       // If the viewmode isn't found, it's not one of our urls. Carry on.
@@ -923,7 +926,7 @@
         value: [
           // Show the sidebar on all places if its not manually shut off or
           // turned into a fullscreen route.
-          { path: '*', callbacks: 'routeSidebarDefault'},
+          { path: '*', callbacks: 'routeDefault'},
           { path: '/*id/', callbacks: 'routeDirectCharmId'},
           { path: '/:viewmode/', callbacks: 'routeView' },
           { path: '/:viewmode/search/', callbacks: 'routeView' },
@@ -952,6 +955,17 @@
       deploy: {},
 
       /**
+        The default viewmode
+
+         @attribute defaultViewmode
+         @default sidebar
+         @type {String}
+       */
+      defaultViewmode: {
+        value: 'sidebar'
+      },
+
+      /**
          @attribute minNode
          @default Node
          @type {Node}

=== modified file 'test/test_browser_app.js'
--- test/test_browser_app.js	2013-07-02 17:10:25 +0000
+++ test/test_browser_app.js	2013-07-03 16:37:36 +0000
@@ -255,29 +255,43 @@
       });
     });
 
-    it('* route set sidebar by default', function() {
-      app = new browser.Browser();
-      // Stub out the sidebar so we don't render anything.
+    it('* route uses the default viewmode', function() {
+      app = new browser.Browser({defaultViewmode: 'sidebar'});
       app.sidebar = function() {};
       var req = {
         path: '/'
       };
 
-      app.routeSidebarDefault(req, null, next);
+      app.routeDefault(req, null, next);
       // The viewmode should be populated now to the default.
       assert.equal(req.params.viewmode, 'sidebar');
+
+      app = new browser.Browser({defaultViewmode: 'fullscreen'});
+      app.fullscreen = function() {};
+      req = {
+        path: '/'
+      };
+      app.routeDefault(req, null, next);
+      assert.equal(req.params.viewmode, 'fullscreen');
     });
 
-    it('prevents * route from doing more than sidebar by default', function() {
-      app = new browser.Browser();
+    it('prevents * route from doing more than the default', function() {
+      app = new browser.Browser({defaultViewmode: 'sidebar'});
       var req = {
         path: '/sidebar'
       };
 
-      app.routeSidebarDefault(req, null, next);
+      app.routeDefault(req, null, next);
       // The viewmode is ignored. This path isn't meant for this route
       // callable to deal with at all.
       assert.equal(req.params, undefined);
+
+      app = new browser.Browser({defaultViewmode: 'fullscreen'});
+      req = {
+        path: '/fullscreen'
+      };
+      app.routeDefault(req, null, next);
+      assert.equal(req.params, undefined);
     });
 
     it('/charm/id routes to a sidebar view correcetly', function() {