Merge lp:~rharding/juju-gui/browser_links into lp:juju-gui/experimental

[Richard Harding]

Support linking through router code for urls.

- Add state tracking to the subapp and use that to determine which UX parts
need calling and which do not.
- Add an event to the subapp and views to handle catching requests to route to
a new url.
- Add tests to walk the paths of rendering to make sure we're updating and
calling correctly.
- Add some stubs for search, but it's to be implemented in follow up.

https://codereview.appspot.com/8726048/

[Richard Harding (rharding)]

Reviewers: mp+159215_code.launchpad.net,

Message:
Please take a look.

Description:

TBD

https://code.launchpad.net/~rharding/juju-gui/browser_links/+merge/159215

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M app/subapps/browser/browser.js
   M app/subapps/browser/templates/browser_charm.handlebars
   M app/subapps/browser/views/charm.js
   M app/subapps/browser/views/editorial.js
   M app/subapps/browser/views/view.js
   M app/templates/browser-search.handlebars
   M app/templates/charm-token.handlebars

[Richard Harding (rharding)]

Please take a look.

https://codereview.appspot.com/8726048/

[lp:~rharding/juju-gui/browser_links]

579. By Richard Harding on 2013-04-17

Garden

[j.c.sackett (jcsackett)]

Rick, several comments below, and really I do think this probably needs
tests to land, since I'm seeing lots of places where the functionality
is pretty complicated.

Also, it seems like we might save ourselves some headache by removing
the "editorial is implicitly the default" notion, and simply having
"editorial" as a path component, and going explicit with everything.
That way we can simply check "is this the editorial view" rather than
checking several aspects of view state.

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

https://codereview.appspot.com/8726048/diff/3001/app/subapps/browser/browser.js#newcode30
app/subapps/browser/browser.js:30: */
Nitpicky, but your comment blocks seem to be misformatted. Shouldn't
there be stars as the leading character the whole way?

/**
   * Some text
   */

https://codereview.appspot.com/8726048/diff/3001/app/subapps/browser/browser.js#newcode94
app/subapps/browser/browser.js:94: this._viewState =
Y.merge(this._oldState, {});
Is this necessary? Can't you just do this._viewState = {...} and skip
merging with {}?

https://codereview.appspot.com/8726048/diff/3001/app/subapps/browser/browser.js#newcode219
app/subapps/browser/browser.js:219: var charmID = req.params.id;
I think this should just be 'charm' or 'charmDetails'; really, we're
just handling the routing here, not the rendering. Just as in fullscreen
and sidebar.

I just spent a bit of time lost, trying to figure out why we hadn't
decided *before* a render question if it was the right thing to do,
before realizing these methods *are* the methods deciding if a given
view is active/needs to be rendered.

https://codereview.appspot.com/8726048/diff/3001/app/subapps/browser/browser.js#newcode260
app/subapps/browser/browser.js:260: renderEditorial: function(req, res,
next) {
Same here.

https://codereview.appspot.com/8726048/diff/3001/app/subapps/browser/browser.js#newcode297
app/subapps/browser/browser.js:297: * Render the fullscreen view to the
client.
These docs are misleading as well--we're not rendering, so much as
deciding if a given view needs to be active/rendered.

https://codereview.appspot.com/8726048/diff/3001/app/subapps/browser/templates/browser_charm.handlebars
File app/subapps/browser/templates/browser_charm.handlebars (right):

https://codereview.appspot.com/8726048/diff/3001/app/subapps/browser/templates/browser_charm.handlebars#newcode4
app/subapps/browser/templates/browser_charm.handlebars:4: <a href=""
class="back">{{#if isFullscreen}}Back{{else}}Close{{/if}}</a>
We're going to be using some helper thing that generates the url here,
right?

https://codereview.appspot.com/8726048/diff/3001/app/subapps/browser/views/editorial.js
File app/subapps/browser/views/editorial.js (right):

https://codereview.appspot.com/8726048/diff/3001/app/subapps/browser/views/editorial.js#newcode68
app/subapps/browser/views/editorial.js:68: *
If container isn't a parameter anymore, we're not loading into a
specified container.

https://codereview.appspot.com/8726048/

[lp:~rharding/juju-gui/browser_links]

580. By Richard Harding on 2013-04-19

Pull in design work, trunk, update conflicts

581. By Richard Harding on 2013-04-19

Pull in sidebar ux adjustments

582. By Richard Harding on 2013-04-19

Tests are good

583. By Richard Harding on 2013-04-19

Add more tests to routing

584. By Richard Harding on 2013-04-19

Lint

[Richard Harding (rharding)]

Please take a look.

https://codereview.appspot.com/8726048/

[Curtis Hovey (sinzui)]

Hi Rick. I have a few questions. Oh, and the remark about setStyle() is
a rant. I don't want this fixed this week.

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

https://codereview.appspot.com/8726048/diff/9001/app/subapps/browser/browser.js#newcode113
app/subapps/browser/browser.js:113: viewmode: undefined,
Why undefined? I expect undefined to be a state that has never been
examined/set. Why isn't null correct?

https://codereview.appspot.com/8726048/diff/9001/app/subapps/browser/browser.js#newcode225
app/subapps/browser/browser.js:225: this._viewState.charmID = undefined;
Again, why define the value to be undefined? I think we mean the value
is null.

https://codereview.appspot.com/8726048/diff/9001/app/subapps/browser/browser.js#newcode391
app/subapps/browser/browser.js:391:
Y.one('#subapp-browser').setStyle('display', 'block');
I think setStyle('display', 'assumed-valid-state') is bad. I see it is
done in several places. We should be adding/removing classes. While
'none' is valid for all html elements, 'block' is only valid for a
subset of elements. The design might need inline or inline-block in the
future to make content visible -- many code changes are needed where I
would prefer huw or myself to make a single style change to a class.

https://codereview.appspot.com/8726048/diff/9001/app/subapps/browser/browser.js#newcode437
app/subapps/browser/browser.js:437: // Else render the editorial
content.
This is not an else. I can render search and editorial according to this
logic.

https://codereview.appspot.com/8726048/

[Richard Harding (rharding)]

Feedback below.

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

https://codereview.appspot.com/8726048/diff/9001/app/subapps/browser/browser.js#newcode113
app/subapps/browser/browser.js:113: viewmode: undefined,
On 2013/04/19 15:36:54, curtis wrote:
> Why undefined? I expect undefined to be a state that has never been
> examined/set. Why isn't null correct?

Null would completely work. I can update it.

https://codereview.appspot.com/8726048/diff/9001/app/subapps/browser/browser.js#newcode225
app/subapps/browser/browser.js:225: this._viewState.charmID = undefined;
On 2013/04/19 15:36:54, curtis wrote:
> Again, why define the value to be undefined? I think we mean the value
is null.

Will change

https://codereview.appspot.com/8726048/diff/9001/app/subapps/browser/browser.js#newcode391
app/subapps/browser/browser.js:391:
Y.one('#subapp-browser').setStyle('display', 'block');
On 2013/04/19 15:36:54, curtis wrote:
> I think setStyle('display', 'assumed-valid-state') is bad. I see it is
done in
> several places. We should be adding/removing classes. While 'none' is
valid for
> all html elements, 'block' is only valid for a subset of elements. The
design
> might need inline or inline-block in the future to make content
visible -- many
> code changes are needed where I would prefer huw or myself to make a
single
> style change to a class.

This is a bit of a temp hack since we're not the default, but out div
exists. I can XXX this to remove the whole thing going forward.

The other thing is that we have the collapsed view that's not here yet
that'll replace the job this is doing. I started to use the .hidden but
that uses visibility so our div is still there and 350px wide causing
click issues on the environment.

https://codereview.appspot.com/8726048/diff/9001/app/subapps/browser/browser.js#newcode437
app/subapps/browser/browser.js:437: // Else render the editorial
content.
On 2013/04/19 15:36:54, curtis wrote:
> This is not an else. I can render search and editorial according to
this logic.

Thanks, missed this in refactors.

https://codereview.appspot.com/8726048/

[lp:~rharding/juju-gui/browser_links]

585. By Richard Harding on 2013-04-19

Fix pre review

[Richard Harding (rharding)]

Please take a look.

https://codereview.appspot.com/8726048/

[Curtis Hovey (sinzui)]

Thank's Rick. LGTM.

https://codereview.appspot.com/8726048/

[Jeff Pihach (hatch)]

LGTM - There are a few rename and documentation requests below but good
work, thanks for working through this!

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

https://codereview.appspot.com/8726048/diff/15001/app/subapps/browser/browser.js#newcode169
app/subapps/browser/browser.js:169: _stateChanged: function(field) {
These 4 methods which return true/false don't actually do what their
method names imply.

if (this._hasStateChanged()) {} would be easier to follow as you can
assume that there are no side effects.

https://codereview.appspot.com/8726048/diff/15001/app/subapps/browser/browser.js#newcode305
app/subapps/browser/browser.js:305: Y.one('.bws-view-data').show();
Where does this element come from? Isn't it being rendered to the DOM
with the render() above?

https://codereview.appspot.com/8726048/diff/15001/app/subapps/browser/browser.js#newcode355
app/subapps/browser/browser.js:355: renderSearchResults: function(req,
res, next) {
linter might complain about this not being documented with @method

https://codereview.appspot.com/8726048/diff/15001/app/subapps/browser/browser.js#newcode379
app/subapps/browser/browser.js:379: if (this._showCharm()) {
See above comments - this looks like it's showing the charm and then
checking for a truthy return value that it's succeeded

https://codereview.appspot.com/8726048/diff/15001/app/subapps/browser/views/editorial.js
File app/subapps/browser/views/editorial.js (right):

https://codereview.appspot.com/8726048/diff/15001/app/subapps/browser/views/editorial.js#newcode171
app/subapps/browser/views/editorial.js:171: renderTo: {
could we document these attrs please.

https://codereview.appspot.com/8726048/diff/15001/app/templates/charm-token.handlebars
File app/templates/charm-token.handlebars (right):

https://codereview.appspot.com/8726048/diff/15001/app/templates/charm-token.handlebars#newcode8
app/templates/charm-token.handlebars:8: <div class="charm-icon"></div>
I think the indentation is off here.

https://codereview.appspot.com/8726048/diff/15001/test/test_browser_app.js
File test/test_browser_app.js (right):

https://codereview.appspot.com/8726048/diff/15001/test/test_browser_app.js#newcode333
test/test_browser_app.js:333: it('sidebar to details does no rerender
sidebar', function() {
what is this doing?

'renders details without re-rendering sidebar' ?

There are a few which aren't clear from their titles

https://codereview.appspot.com/8726048/

[lp:~rharding/juju-gui/browser_links]

586. By Richard Harding on 2013-04-19

Updates per code review

[Richard Harding (rharding)]

*** Submitted:

Support linking through router code for urls.

- Add state tracking to the subapp and use that to determine which UX
parts
need calling and which do not.
- Add an event to the subapp and views to handle catching requests to
route to
a new url.
- Add tests to walk the paths of rendering to make sure we're updating
and
calling correctly.
- Add some stubs for search, but it's to be implemented in follow up.

R=j.c.sackett, curtis, jeff.pihach
CC=
https://codereview.appspot.com/8726048

https://codereview.appspot.com/8726048/

=== modified file 'app/subapps/browser/browser.js'
--- app/subapps/browser/browser.js	2013-04-17 22:57:48 +0000
+++ app/subapps/browser/browser.js	2013-04-19 17:11:24 +0000
@@ -21,45 +21,11 @@
    */
   ns.Browser = Y.Base.create('subapp-browser', Y.juju.SubApp, [], {
     /**
-     * Some routes might have sub parts that hint to where a user wants focus.
-     * In particular we've got the tabs that might have focus. They are the
-     * last optional component of some of the routes.
-     *
-     * @method _getSubPath
-     * @param {String} path the full path to search for the sub path.
-     *
-     */
-    _getSubPath: function(path) {
-      var reLastWord = /[^\/]*\/?$/,
-          lastWords = path.match(reLastWord);
-      if (lastWords.length) {
-        return lastWords[0].replace('/', '');
-      } else {
-        return undefined;
-      }
-    },
-
-    /**
-     * Generate a standard shared set of cfg all Views can expect to see.
-     *
-     * @method _getViewCfg
-     * @param {Object} cfg additional config to merge into the default view
-     * config.
-     *
-     */
-    _getViewCfg: function(cfg) {
-      return Y.merge(cfg, {
-        db: this.get('db'),
-        store: this.get('store')
-      });
-    },
-
-    /**
-     * Show or hide the details panel.
-     *
-     * @method _detailsVisible
-     * @param {Boolean} visible set the panel to hide or show.
-     *
+        Show or hide the details panel.
+
+        @method _detailsVisible
+        @param {Boolean} visible set the panel to hide or show.
+
      */
     _detailsVisible: function(visible) {
       var detailsNode = Y.one('.bws-view-data'),
@@ -78,15 +44,183 @@
     },
 
     /**
+        Given the current subapp state, generate a url to pass up to the
+        routing code to route to.
+
+        @method _getStateUrl
+        @param {Object} change the values to change in the current state.
+
+     */
+    _getStateUrl: function(change) {
+      var urlParts = ['/bws'];
+      this._oldState = this._viewState;
+      this._viewState = Y.merge(this._viewState, change);
+
+      urlParts.push(this._viewState.viewmode);
+      if (this._viewState.search) {
+        urlParts.push('search');
+      }
+      if (this._viewState.charmID) {
+        urlParts.push(this._viewState.charmID);
+      }
+
+      // Always end on a /
+      return urlParts.join('/');
+    },
+
+    /**
+     * Generate a standard shared set of cfg all Views can expect to see.
+     *
+     * @method _getViewCfg
+     * @param {Object} cfg additional config to merge into the default view
+     * config.
+     *
+     */
+    _getViewCfg: function(cfg) {
+      return Y.merge(cfg, {
+        db: this.get('db'),
+        store: this.get('store')
+      });
+    },
+
+    /**
+       Create an initial subapp state for later url generation.
+
+       @method _initState
+
+     */
+    _initState: function() {
+      this._oldState = {
+        viewmode: null,
+        search: null,
+        charmID: null
+      };
+      this._viewState = Y.merge(this._oldState, {});
+    },
+
+    /**
+      Determine if we should render the charm details based on the current
+      state.
+
+      @return {Boolean} true if should show.
+
+    */
+    _shouldShowCharm: function() {
+      if (
+          this._viewState.charmID &&
+          (
+           this._hasStateChanged('charmID') ||
+           this._hasStateChanged('viewmode')
+          )
+      ) {
+        return true;
+      } else {
+        return false;
+      }
+    },
+
+    /**
+      Determine if we should render the editorial content based on the current
+      state.
+
+      @return {Boolean} true if should show.
+
+    */
+    _shouldShowEditorial: function() {
+      if (
+          !this._viewState.search &&
+          this._hasStateChanged('viewmode')
+      ) {
+        return true;
+      } else {
+        return false;
+      }
+    },
+
+    /**
+      Determine if we should render the search results based on the current
+      state.
+
+      @return {Boolean} true if should show.
+
+    */
+    _shouldShowSearch: function() {
+      if (
+          this._viewState.search &&
+          (
+           this._hasStateChanged('search') ||
+           this._hasStateChanged('viewmode') ||
+           this._hasStateChanged('querystring')
+          )
+      ) {
+        return true;
+      } else {
+        return false;
+      }
+    },
+
+    /**
+      Verify that a particular part of the state has changed.
+
+      @method _hasStateChanged
+      @param {String} field the part of the state to check.
+
+     */
+    _hasStateChanged: function(field) {
+      if (this._oldState[field] === this._viewState[field]) {
+        return false;
+      } else {
+        return true;
+      }
+    },
+
+    /**
+      Update the oldState with the viewState now that we're done processing
+      the request.
+
+      @method _saveState
+
+     */
+    _saveState: function() {
+      this._oldState = Y.merge(
+          this._oldState,
+          this._viewState);
+    },
+
+    /**
+       Given the params in the route determine what the new state is going to
+       be.
+
+       @method _updateState
+       @param {String} path the requested path.
+       @param {Object} params the params from the request payload.
+
+     */
+    _updateState: function(path, params) {
+      // Update the viewmode. Every request has a viewmode.
+      this._viewState.viewmode = params.viewmode;
+
+      // Check for a charm id in the request.
+      if (params.id) {
+        this._viewState.charmID = params.id;
+      } else {
+        this._viewState.charmID = null;
+      }
+
+      // Check for search in the request.
+      if (path.indexOf('search') !== -1) {
+        this._viewState.search = true;
+      } else {
+        this._viewState.search = false;
+      }
+    },
+
+    /**
      * The available Views run from this sub app.
      * @attribute views
      *
      */
     views: {
-      charmDetails: {
-        type: 'juju.browser.views.BrowserCharmView',
-        preserve: false
-      },
       fullscreen: {
         type: 'juju.browser.views.FullScreen',
         preserve: false
@@ -105,6 +239,7 @@
      */
     destructor: function() {
       this._cacheCharms.destroy();
+      delete this._viewState;
     },
 
     /**
@@ -118,34 +253,53 @@
       // Hold onto charm data so we can pass model instances to other views when
       // charms are selected.
       this._cacheCharms = new models.BrowserCharmList();
+      this._initState();
+
+      // Listen for navigate events from any views we're rendering.
+      this.on('*:viewNavigate', function(ev) {
+        var url;
+        if (ev.url) {
+          url = ev.url;
+        } else if (ev.change) {
+          url = this._getStateUrl(ev.change);
+        }
+        this.navigate(url);
+      });
     },
 
     /**
-     * Render the fullscreen view to the client.
+     * Render the sidebar view of a specific charm to the client.
      *
-     * @method fullscreen
+     * @method sidebarCharm
      * @param {Request} req current request object.
      * @param {Response} res current response object.
      * @param {function} next callable for the next route in the chain.
      *
      */
-    fullscreen: function(req, res, next) {
-      this.get('container').setStyle('display', 'block');
-
-      if (!this._fullscreen) {
-        this._fullscreen = this.showView('fullscreen', this._getViewCfg(), {
-          'callback': function(view) {
-            // if the fullscreen isn't the last part of the path, then ignore
-            // the editorial content.
-            if (this._getSubPath(req.path) === 'fullscreen') {
-              this.renderEditorial(req, res, next);
-            }
-            next();
-          }
-        });
-      } else {
-        next();
-      }
+    renderCharmDetails: function(req, res, next) {
+      var charmID = req.params.id;
+      var extraCfg = {
+        charmID: charmID,
+        container: Y.Node.create('<div class="charmview"/>')
+      };
+
+      // The details view needs to know if we're using a fullscreen template
+      // or the sidebar version.
+      if (this._viewState.viewmode === 'fullscreen') {
+        extraCfg.isFullscreen = true;
+      }
+
+      // Gotten from the sidebar creating the cache.
+      var model = this._cacheCharms.getById(charmID);
+
+      if (model) {
+        extraCfg.charm = model;
+      }
+
+      this._details = new Y.juju.browser.views.BrowserCharmView(
+          this._getViewCfg(extraCfg));
+      this._details.render();
+      this._details.addTarget(this);
     },
 
     /**
@@ -161,33 +315,79 @@
 
      */
     renderEditorial: function(req, res, next) {
+      // If loading the interesting content then it's not a search going on.
       var container = this.get('container'),
           editorialContainer,
           extraCfg = {};
 
-      if (req.path.indexOf('fullscreen') !== -1) {
+      if (this._viewState.viewmode === 'fullscreen') {
         // The fullscreen view requires that there be no editorial content if
         // we're looking at a specific charm. The div we dump our content into
         // is shared. So if the url is /fullscreen show editorial content, but
         // if it's not, there's something else handling displaying the
         // view-data.
-        editorialContainer = container.one(' .bws-view-data');
+        extraCfg.renderTo = container.one('.bws-view-data');
         extraCfg.isFullscreen = true;
       } else {
         // If this is the sidebar view, then the editorial content goes into a
         // different div since we can view both editorial content and
         // view-data (such as a charm details) side by side.
-        editorialContainer = container.one('.bws-content');
-      }
-
-      if (!this._editorial) {
-        this._editorial = new Y.juju.browser.views.EditorialView(
-            this._getViewCfg(extraCfg));
-
-        this._editorial.render(editorialContainer);
-        // Add any sidebar charms to the running cache.
-        this._cacheCharms.add(this._editorial._cacheCharms);
-      }
+        extraCfg.renderTo = container.one('.bws-content');
+      }
+
+      this._editorial = new Y.juju.browser.views.EditorialView(
+          this._getViewCfg(extraCfg));
+
+      this._editorial.render();
+      this._editorial.addTarget(this);
+
+      // Add any sidebar charms to the running cache.
+      this._cacheCharms.add(this._editorial._cacheCharms);
+    },
+
+    /**
+      Place holder for a method to render out search so we can test url parsing
+
+    */
+    renderSearchResults: function(req, res, next) {
+      console.log('rendered search results.');
+    },
+
+    /**
+     * Render the fullscreen view to the client.
+     *
+     * @method fullscreen
+     * @param {Request} req current request object.
+     * @param {Response} res current response object.
+     * @param {function} next callable for the next route in the chain.
+     *
+     */
+    fullscreen: function(req, res, next) {
+      // If we've switched to viewmode fullscreen, we need to render it.
+      // We know the viewmode is already fullscreen because we're in this
+      // function.
+      if (this._hasStateChanged('viewmode')) {
+        Y.one('#subapp-browser').setStyle('display', 'block');
+        this._fullscreen = this.showView('fullscreen', this._getViewCfg());
+      }
+
+      // If we've changed the charmID or the viewmode has changed and we have
+      // a charmID, render charmDetails.
+      if (this._shouldShowCharm()) {
+        this._detailsVisible(true);
+        this.renderCharmDetails(req, res, next);
+      } else if (this._shouldShowSearch()) {
+        // Render search results if search is in the url and the viewmode or
+        // the search has been changed in the state.
+        this.renderSearchResults(req, res, next);
+      } else {
+        // Else render the editorial content.
+        this.renderEditorial(req, res, next);
+      }
+
+      // Sync that the state has changed.
+      this._saveState();
+      next();
     },
 
     /**
@@ -200,72 +400,61 @@
      *
      */
     sidebar: function(req, res, next) {
-      this.get('container').setStyle('display', 'block');
-      // Clean up any details we've got.
-      if (this._details) {
-        this._details.destroy({remove: true});
+      // If we've switched to viewmode sidebar, we need to render it.
+      if (this._hasStateChanged('viewmode')) {
+        Y.one('#subapp-browser').setStyle('display', 'block');
+        this._sidebar = this.showView('sidebar', this._getViewCfg());
+      }
+
+      // Render search results if search is in the url and the viewmode or the
+      // search has been changed in the state.
+      if (this._shouldShowSearch()) {
+        this.renderSearchResults(req, res, next);
+      }
+
+      if (this._shouldShowEditorial()) {
+        this.renderEditorial(req, res, next);
+      }
+
+      // If we've changed the charmID or the viewmode has changed and we have
+      // a charmID, render charmDetails.
+      if (this._shouldShowCharm()) {
+        this._detailsVisible(true);
+        this.renderCharmDetails(req, res, next);
       }
 
       // If the sidebar is the final part of the route, then hide the div for
       // viewing the charm details.
-      if (this._getSubPath(req.path) === 'sidebar') {
+      if (!this._viewState.charmID) {
         this._detailsVisible(false);
+        var detailsNode = Y.one('.bws-view-data');
+        if (detailsNode) {
+          detailsNode.hide();
+        }
+        // Clean up any details we've got.
+        if (this._details) {
+          this._details.destroy({remove: true});
+        }
       }
 
-      if (!this._sidebar) {
-        // Whenever the sidebar view is rendered it needs some editorial
-        // content to display to the user. We only need once instance though,
-        // so only render it on the first view. As users click on charm to
-        // charm and we generate urls /sidebar/precise/xxx we don't want to
-        // re-render the sidebar content.
-        this._sidebar = this.showView('sidebar', this._getViewCfg(), {
-          'callback': function(view) {
-            this.renderEditorial(req, res, next);
-            next();
-          }
-        });
-      } else {
-        next();
-      }
+      // Sync that the state has changed.
+      this._saveState();
+      next();
     },
 
     /**
-     * Render the sidebar view of a specific charm to the client.
-     *
-     * @method sidebarCharm
-     * @param {Request} req current request object.
-     * @param {Response} res current response object.
-     * @param {function} next callable for the next route in the chain.
-     *
+        Dispatch to the correct viewmode based on the route that was hit.
+
+       @method routeView
+       @param {Request} req current request object.
+       @param {Response} res current response object.
+       @param {function} next callable for the next route in the chain.
+
      */
-    charmDetails: function(req, res, next) {
-      var charmID = req.params.id;
-      var extraCfg = {
-        charmID: charmID,
-        container: Y.Node.create('<div class="charmview"/>')
-      };
-
-      // The details view needs to know if we're using a fullscreen template
-      // or the sidebar version.
-      if (req.path.indexOf('fullscreen') !== -1) {
-        extraCfg.isFullscreen = true;
-      }
-
-      // Gotten from the sidebar creating the cache.
-      var model = this._cacheCharms.getById(charmID);
-
-      if (model) {
-        extraCfg.charm = model;
-      }
-
-      this._details = new Y.juju.browser.views.BrowserCharmView(
-          this._getViewCfg(extraCfg));
-      this._details.render();
-      // Make sure we show the bws-view-data div that the details renders
-      // into.
-      this._detailsVisible(true);
-
-      next();
+    routeView: function(req, res, next) {
+      // Update the state for the rest of things to figure out what to do.
+      this._updateState(req.path, req.params);
+      this[req.params.viewmode](req, res, next);
     }
 
   }, {
@@ -318,13 +507,10 @@
       routes: {
         value: [
           // Double routes are needed to catch /fullscreen and /fullscreen/
-          { path: '/bws/fullscreen/', callbacks: 'fullscreen' },
-          { path: '/bws/fullscreen/*/', callbacks: 'fullscreen' },
-          { path: '/bws/fullscreen/*id/', callbacks: 'charmDetails' },
-
-          { path: '/bws/sidebar/', callbacks: 'sidebar' },
-          { path: '/bws/sidebar/*/', callbacks: 'sidebar' },
-          { path: '/bws/sidebar/*id/', callbacks: 'charmDetails' }
+          { path: '/bws/:viewmode/', callbacks: 'routeView' },
+          { path: '/bws/:viewmode/search/', callbacks: 'routeView' },
+          { path: '/bws/:viewmode/search/*id/', callbacks: 'routeView' },
+          { path: '/bws/:viewmode/*id/', callbacks: 'routeView' }
         ]
       },
 

=== modified file 'app/subapps/browser/templates/browser_charm.handlebars'
--- app/subapps/browser/templates/browser_charm.handlebars	2013-04-19 06:29:32 +0000
+++ app/subapps/browser/templates/browser_charm.handlebars	2013-04-19 17:11:24 +0000
@@ -1,7 +1,7 @@
 <div class="charm yui3-g">
     <div class="content yui3-u-{{#if isFullscreen}}5-6{{else}}1{{/if}}">
         <div class="nav">
-            <a href="/bws/sidebar/" class="back">{{#if isFullscreen}}Back{{else}}Close{{/if}}</a>
+            <a href="" class="back">{{#if isFullscreen}}Back{{else}}Close{{/if}}</a>
             <a href="" class="add">Add</a>
             <a href="" class="share">
                 <img src="/juju-ui/assets/images/share_icon.jpg" alt="Share" />

=== modified file 'app/subapps/browser/views/charm.js'
--- app/subapps/browser/views/charm.js	2013-04-18 19:07:41 +0000
+++ app/subapps/browser/views/charm.js	2013-04-19 17:11:24 +0000
@@ -38,6 +38,9 @@
       },
       '#bws-hooks select': {
         change: '_loadHookContent'
+      },
+      '.nav .back': {
+        click: '_handleBack'
       }
     },
 
@@ -194,6 +197,23 @@
     },
 
     /**
+        Handle the back button being clicked on from the header of the
+        details.
+
+        @method _handleBack
+        @param {Event} ev the click event handler.
+
+     */
+    _handleBack: function(ev) {
+      ev.halt();
+      this.fire('viewNavigate', {
+        change: {
+          charmID: null
+        }
+      });
+    },
+
+    /**
      * Determine which intro copy to display depending on the number
      * of interfaces.
      *
@@ -242,7 +262,6 @@
           build += string;
         }
       });
-
       interfaceIntro[build] = true;
       return interfaceIntro;
     },

=== modified file 'app/subapps/browser/views/editorial.js'
--- app/subapps/browser/views/editorial.js	2013-04-17 01:09:21 +0000
+++ app/subapps/browser/views/editorial.js	2013-04-19 17:11:24 +0000
@@ -25,6 +25,31 @@
   ns.EditorialView = Y.Base.create('browser-view-sidebar', Y.View, [], {
     template: views.Templates.editorial,
 
+    events: {
+      '.charm-token': {
+        click: '_handleCharmSelection'
+      }
+    },
+
+    /**
+        When selecting a charm from the list make sure we re-route the app to
+        the details view with that charm selected.
+
+        @method _handleCharmSelection
+        @param {Event} ev the click event handler for the charm selected.
+
+     */
+    _handleCharmSelection: function(ev) {
+      var charm = ev.currentTarget;
+      var charmID = charm.getData('charmid');
+
+      this.fire('viewNavigate', {
+        change: {
+          charmID: charmID
+        }
+      });
+    },
+
     /**
      * General YUI initializer.
      *
@@ -45,17 +70,11 @@
      * @param {Node} container An optional node to override where it's going.
      *
      */
-    render: function(container) {
+    render: function() {
       var tpl = this.template(this.getAttrs()),
           tplNode = Y.Node.create(tpl),
           store = this.get('store');
 
-      if (typeof container !== 'object') {
-        container = this.get('container');
-      } else {
-        this.set('container', container);
-      }
-
       // By default we grab the editorial content from the api to use for
       // display.
       this.get('store').interesting({
@@ -96,7 +115,9 @@
           });
           newCharmContainer.render(newContainer);
 
-          container.setHTML(tplNode);
+          var container = this.get('container');
+          container.append(tplNode);
+          this.get('renderTo').setHTML(container);
 
           // Add the charms to the cache for use in other views.
           // Start with a reset to empty any current cached models.
@@ -144,9 +165,37 @@
     }
   }, {
     ATTRS: {
+      /**
+       * Is this rendering of the editorial view for fullscreen or sidebar
+       * purposes?
+       *
+       * @attribute isFullscreen
+       * @default false
+       * @type {Boolean}
+       *
+       */
       isFullscreen: {
         value: false
       },
+      /**
+       * What is the container node we should render our container into?
+       *
+       * @attribute renderTo
+       * @default undefined
+       * @type {Node}
+       *
+       */
+      renderTo: {
+
+      },
+      /**
+       * The Charmworld0 Api store instance for loading content.
+       *
+       * @attribute store
+       * @default undefined
+       * @type {Charmworld0}
+       *
+       */
       store: {
 
       }

=== modified file 'app/subapps/browser/views/view.js'
--- app/subapps/browser/views/view.js	2013-04-12 19:53:11 +0000
+++ app/subapps/browser/views/view.js	2013-04-19 17:11:24 +0000
@@ -76,9 +76,13 @@
           this.search.on(
               this.search.EVT_TOGGLE_VIEWABLE, this._toggleBrowser, this)
       );
+
+      this.addEvent(
+          this.search.on(
+              this.search.EVT_TOGGLE_FULLSCREEN, this._toggleFullscreen, this)
+      );
     },
 
-
     /**
      * Render out the main search widget and controls shared across various
      * views.
@@ -116,6 +120,7 @@
      *
      */
     _toggleBrowser: function(ev) {
+      ev.halt();
       var sidebar = Y.one('.charmbrowser');
 
       if (this.visible) {
@@ -127,6 +132,23 @@
       }
     },
 
+    /**
+        Upon clicking the fullscreen toggle icon make sure we re-route to the
+        new form of the UX.
+
+        @method _toggleFullscreen
+        @param {Event} ev the click event handler on the button.
+
+     */
+    _toggleFullscreen: function(ev) {
+      ev.halt();
+      var change = {
+        viewmode: this.isFullscreen() ? 'sidebar' : 'fullscreen'
+      };
+      this.fire('viewNavigate', {
+        change: change
+      });
+    },
 
     /**
      * Shared method to generate a message to the user based on a bad api

=== modified file 'app/templates/browser-search.handlebars'
--- app/templates/browser-search.handlebars	2013-04-05 13:10:59 +0000
+++ app/templates/browser-search.handlebars	2013-04-19 17:11:24 +0000
@@ -11,9 +11,7 @@
         </form>
     </div>
     <div class="toggle-fullscreen">
-        <a href="{{ fullscreenTarget }}">
-            <img src="/juju-ui/assets/images/browser_minimize.jpg"
-                alt="Minimize" />
-        </a>
+        <img src="/juju-ui/assets/images/browser_minimize.jpg"
+            alt="Minimize" />
     </div>
 </div>

=== modified file 'app/templates/charm-token.handlebars'
--- app/templates/charm-token.handlebars	2013-04-19 06:29:32 +0000
+++ app/templates/charm-token.handlebars	2013-04-19 17:11:24 +0000
@@ -1,12 +1,21 @@
 <a href="/bws/sidebar/{{id}}"
     class="charm-token yui3-g"
     data-charmid="{{id}}">
+<<<<<<< TREE
     <span class="yui3-u">
         {{#if iconfile }}
             <img src="data:image/svg;base64,{{ iconfile }}" alt="{{ name }} icon">
         {{else}}
            <div class="charm-icon"></div>
         {{/if}}
+=======
+    <span class="yui3-u">
+        {{#if iconfile }}
+            <img src="data:image/svg;base64,{{ iconfile }}" alt="{{ name }} icon">
+        {{else}}
+            <div class="charm-icon"></div>
+        {{/if}}
+>>>>>>> MERGE-SOURCE
     </span>
     <span class="yui3-u-3-4">
         <span class="title">

=== modified file 'test/test_browser_app.js'
--- test/test_browser_app.js	2013-04-15 16:19:55 +0000
+++ test/test_browser_app.js	2013-04-19 17:11:24 +0000
@@ -156,16 +156,286 @@
       });
     });
 
-    it('should be able to determine if the route is a sub path', function() {
-      var app = new browser.Browser(),
-          subpaths = ['configuration', 'hooks', 'interfaces', 'qa', 'readme'];
-
-      Y.Array.each(subpaths, function(path) {
-        var url = '/bws/fullscreen/charm/id/stuff/' + path + '/';
-        app._getSubPath(url).should.eql(path);
-      });
-    });
-
-  });
-
+  });
+
+  describe('browser subapp display tree', function() {
+    var Y, browser, hits, ns, resetHits;
+
+    before(function(done) {
+      Y = YUI(GlobalConfig).use(
+          'app-subapp-extension',
+          'juju-views',
+          'juju-browser',
+          'subapp-browser', function(Y) {
+            browser = Y.namespace('juju.subapps');
+
+            resetHits = function() {
+              hits = {
+                fullscreen: false,
+                sidebar: false,
+                renderCharmDetails: false,
+                renderEditorial: false,
+                renderSearchResults: false
+              };
+            };
+            done();
+          });
+    });
+
+    before(function(done) {
+      Y = YUI(GlobalConfig).use(
+          'juju-views',
+          'juju-browser',
+          'subapp-browser', function(Y) {
+            ns = Y.namespace('juju.subapps');
+            done();
+          });
+    });
+
+    beforeEach(function() {
+      var docBody = Y.one(document.body);
+      Y.Node.create('<div id="subapp-browser">' +
+          '</div>').appendTo(docBody);
+
+      // Track which render functions are hit.
+      resetHits();
+
+      // Mock out a dummy location for the Store used in view instances.
+      window.juju_config = {
+        charmworldURL: 'http://localhost'
+      };
+
+      browser = new ns.Browser();
+      // Block out each render target so we only track it was hit.
+      browser.renderCharmDetails = function() {
+        hits.renderCharmDetails = true;
+      };
+      browser.renderEditorial = function() {
+        hits.renderEditorial = true;
+      };
+      browser.renderSearchResults = function() {
+        hits.renderSearchResults = true;
+      };
+      // showView needs to be hacked because it does the rendering of
+      // fullscreen/sidebar.
+      browser.showView = function(view) {
+        hits[view] = true;
+      };
+    });
+
+    afterEach(function() {
+      browser.destroy();
+      Y.one('#subapp-browser').remove(true);
+    });
+
+    it('bws-sidebar dispatches correctly', function() {
+      var req = {
+        path: '/bws/sidebar/',
+        params: {
+          viewmode: 'sidebar'
+        }
+      };
+      var expected = Y.merge(hits, {
+        sidebar: true,
+        renderEditorial: true
+      });
+
+      browser.routeView(req, undefined, function() {});
+      assert.deepEqual(hits, expected);
+    });
+
+    it('bws-sidebar-charmid dispatches correctly', function() {
+      var req = {
+        path: '/bws/sidebar/precise/apache2-2',
+        params: {
+          viewmode: 'sidebar',
+          id: 'precise/apache2-2'
+        }
+      };
+      var expected = Y.merge(hits, {
+        sidebar: true,
+        renderEditorial: true,
+        renderCharmDetails: true
+      });
+
+      browser.routeView(req, undefined, function() {});
+      assert.deepEqual(hits, expected);
+    });
+
+    it('bws-sidebar-search-charmid dispatches correctly', function() {
+      var req = {
+        path: '/bws/sidebar/search/precise/apache2-2',
+        params: {
+          viewmode: 'sidebar',
+          id: 'precise/apache2-2'
+        }
+      };
+      var expected = Y.merge(hits, {
+        sidebar: true,
+        renderSearchResults: true,
+        renderCharmDetails: true
+      });
+
+      browser.routeView(req, undefined, function() {});
+      assert.deepEqual(hits, expected);
+    });
+
+    it('bws-fullscreen dispatches correctly', function() {
+      var req = {
+        path: '/bws/fullscreen/',
+        params: {
+          viewmode: 'fullscreen'
+        }
+      };
+      var expected = Y.merge(hits, {
+        fullscreen: true,
+        renderEditorial: true
+      });
+
+      browser.routeView(req, undefined, function() {});
+      assert.deepEqual(hits, expected);
+    });
+
+    it('fullscreen-charmid dispatches correctly', function() {
+      var req = {
+        path: '/bws/fullscreen/precise/apache2-2',
+        params: {
+          viewmode: 'fullscreen',
+          id: 'precise/apache2-2'
+        }
+      };
+      var expected = Y.merge(hits, {
+        fullscreen: true,
+        renderCharmDetails: true
+      });
+
+      browser.routeView(req, undefined, function() {});
+      assert.deepEqual(hits, expected);
+    });
+
+    it('fullscreen-search-charmid dispatches correctly', function() {
+      var req = {
+        path: '/bws/fullscreen/search/precise/apache2-2',
+        params: {
+          viewmode: 'fullscreen',
+          id: 'precise/apache2-2'
+        }
+      };
+      var expected = Y.merge(hits, {
+        fullscreen: true,
+        renderCharmDetails: true
+      });
+
+      browser.routeView(req, undefined, function() {});
+      assert.deepEqual(hits, expected);
+    });
+
+    it('sidebar to sidebar-charmid dispatches correctly', function() {
+      var req = {
+        path: '/bws/sidebar/',
+        params: {
+          viewmode: 'sidebar'
+        }
+      };
+      browser.routeView(req, undefined, function() {});
+
+      // Now route through to the charmid from here and we should not hit the
+      // editorial content again.
+      resetHits();
+      req = {
+        path: '/bws/sidebar/precise/apache2-2',
+        params: {
+          viewmode: 'sidebar',
+          id: 'precise/apache2-2'
+        }
+      };
+
+      var expected = Y.merge(hits, {
+        renderCharmDetails: true
+      });
+
+      browser.routeView(req, undefined, function() {});
+      assert.deepEqual(hits, expected);
+    });
+
+    it('sidebar-details to sidebar dispatchse correctly', function() {
+      var req = {
+        path: '/bws/sidebar/precise/apache2-2',
+        params: {
+          viewmode: 'sidebar',
+          id: 'precise/apache2-2'
+        }
+      };
+      browser.routeView(req, undefined, function() {});
+
+      // Reset the hits and we should not redraw anything to update the view.
+      resetHits();
+      req = {
+        path: '/bws/sidebar/',
+        params: {
+          viewmode: 'sidebar'
+        }
+      };
+
+      var expected = Y.merge(hits, {});
+      browser.routeView(req, undefined, function() {});
+      assert.deepEqual(hits, expected);
+    });
+
+    it('fullscreen to fullscreen-details dispatches correctly', function() {
+      var req = {
+        path: '/bws/fullscreen/',
+        params: {
+          viewmode: 'fullscreen'
+        }
+      };
+      browser.routeView(req, undefined, function() {});
+
+      // Now route through to the charmid from here and we should not hit the
+      // editorial content again.
+      resetHits();
+      req = {
+        path: '/bws/fullscreen/precise/apache2-2',
+        params: {
+          viewmode: 'fullscreen',
+          id: 'precise/apache2-2'
+        }
+      };
+
+      var expected = Y.merge(hits, {
+        renderCharmDetails: true
+      });
+
+      browser.routeView(req, undefined, function() {});
+      assert.deepEqual(hits, expected);
+    });
+
+    it('fullscreen-details to fullscreen renders editorial', function() {
+      var req = {
+        path: '/bws/fullscreen/precise/apache2-2',
+        params: {
+          viewmode: 'fullscreen',
+          id: 'precise/apache2-2'
+        }
+      };
+      browser.routeView(req, undefined, function() {});
+
+      // Reset the hits and we should not redraw anything to update the view.
+      resetHits();
+      req = {
+        path: '/bws/fullscreen/',
+        params: {
+          viewmode: 'fullscreen'
+        }
+      };
+
+      var expected = Y.merge(hits, {
+        renderEditorial: true
+      });
+      browser.routeView(req, undefined, function() {});
+      assert.deepEqual(hits, expected);
+    });
+
+  });
 })();
+