Support linking through router code for urls.

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://code.launchpad.net/~rharding/juju-gui/browser_links/+merge/159215

(do not edit description out of merge proposal)

[rharding, 2013-04-17 19:10:57]

Please take a look.

[rharding, 2013-04-17 19:20:23]

Please take a look.

[j.c.sackett, 2013-04-17 21:49:22]

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....
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....
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....
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....
app/subapps/browser/browser.js:260: renderEditorial: function(req, res, next) {
Same here.

https://codereview.appspot.com/8726048/diff/3001/app/subapps/browser/browser....
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/template...
File app/subapps/browser/templates/browser_charm.handlebars (right):

https://codereview.appspot.com/8726048/diff/3001/app/subapps/browser/template...
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/ed...
File app/subapps/browser/views/editorial.js (right):

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

[rharding, 2013-04-19 14:57:58]

Please take a look.

[curtis, 2013-04-19 15:36:54]

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

[rharding, 2013-04-19 15:43:23]

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

[rharding, 2013-04-19 15:55:39]

Please take a look.

[curtis, 2013-04-19 15:58:13]

Thank's Rick. LGTM.

[jeff.pihach, 2013-04-19 16:29:45]

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...
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...
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...
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...
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/e...
File app/subapps/browser/views/editorial.js (right):

https://codereview.appspot.com/8726048/diff/15001/app/subapps/browser/views/e...
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.h...
File app/templates/charm-token.handlebars (right):

https://codereview.appspot.com/8726048/diff/15001/app/templates/charm-token.h...
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#ne...
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

[rharding, 2013-04-19 17:13:56]

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