Code review comment for lp:~jcsackett/juju-gui/abstract-default-viewmode

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 @@

        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('default_viewmode') ||
+ 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('default_viewmode');
        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('default_viewmode');
        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('default_viewmode');
        }

        // 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 default_viewmode
+ @default sidebar
+ @type {String}
+ */
+ default_viewmode: {
+ value: 'sidebar'
+ },
+
+ /**
           @attribute minNode
           @default Node
           @type {Node}