Add binding ui to switch fullscreen v sidebar.

Add binding ui to switch fullscreen v sidebar.

- Update the CSS so fullsreen shows something like fullscreen
- Update the link to toggle in the search widget
- Update the search widget to take the link to use for the toggle since we
can't fire it programatically
- Start process of building a common View class for the sidebar/fullscreen to
extend. Will clean up more as we add more functionality.

https://code.launchpad.net/~rharding/juju-gui/ux_hookups/+merge/155250

(do not edit description out of merge proposal)

[rharding, 2013-03-25 17:36:23]

Please take a look.

[j.c.sackett, 2013-03-25 18:31:52]

This looks good; I have a few questions where it looks like the code is breaking
with conventions and I don't understand why.

https://codereview.appspot.com/7796047/diff/1/app/store/charm.js
File app/store/charm.js (right):

https://codereview.appspot.com/7796047/diff/1/app/store/charm.js#newcode227
app/store/charm.js:227: var res = this._makeRequest('sidebar_editorial',
callbacks);
Are we changing style rules, or is this because this is just a string, not an
actual var?

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

https://codereview.appspot.com/7796047/diff/1/app/subapps/browser/views/view....
app/subapps/browser/views/view.js:29: 
I'm used to seeing things like events and templates in this block, but shouldn't
things like visible and targets live in ATTRs?

https://codereview.appspot.com/7796047/diff/1/app/subapps/browser/views/view....
app/subapps/browser/views/view.js:105: 'apiHost':
window.juju_config.charmworldURL
Should this be altered so a store can be passed in, and the set is only called
if one isn't? That seems to be the more consistent approach.

[jeff.pihach, 2013-03-25 18:33:15]

LGTM Thanks for this - all looks good I just have a couple documentation
requests if you could before it lands.

https://codereview.appspot.com/7796047/diff/1/app/store/charm.js
File app/store/charm.js (right):

https://codereview.appspot.com/7796047/diff/1/app/store/charm.js#newcode227
app/store/charm.js:227: var res = this._makeRequest('sidebar_editorial',
callbacks);
Isn't this going the wrong way ;)

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

https://codereview.appspot.com/7796047/diff/1/app/subapps/browser/views/view....
app/subapps/browser/views/view.js:26: _fullscreenTarget: '',
Could you document these properties - especially noting which ones are expected
to be overwritten in the children.

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

https://codereview.appspot.com/7796047/diff/1/test/test_browser_app.js#newcode29
test/test_browser_app.js:29: charmworldURL: 'http://localhost'
Maybe just make a quick note as to why you're doing this.

[rharding, 2013-03-25 19:19:41]

Thanks for the reviews. Updates coming. Comments below.

https://codereview.appspot.com/7796047/diff/1/app/store/charm.js
File app/store/charm.js (right):

https://codereview.appspot.com/7796047/diff/1/app/store/charm.js#newcode227
app/store/charm.js:227: var res = this._makeRequest('sidebar_editorial',
callbacks);
On 2013/03/25 18:31:52, j.c.sackett wrote:
> Are we changing style rules, or is this because this is just a string, not an
> actual var?
This is a drive by fix. It's not a variable but the actual url path sent to the
server which is not camel cased since it's in Python :)

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

https://codereview.appspot.com/7796047/diff/1/app/subapps/browser/views/view....
app/subapps/browser/views/view.js:26: _fullscreenTarget: '',
On 2013/03/25 18:33:16, jeff.pihach wrote:
> Could you document these properties - especially noting which ones are
expected
> to be overwritten in the children.

rgr, adding docs.

https://codereview.appspot.com/7796047/diff/1/app/subapps/browser/views/view....
app/subapps/browser/views/view.js:29: 
On 2013/03/25 18:31:52, j.c.sackett wrote:
> I'm used to seeing things like events and templates in this block, but
shouldn't
> things like visible and targets live in ATTRs?

There's not currently any reason for these to live in ATTRS. ATTRS are more
heavy weight but allow for things like hooking into change events, dumping to
templates (this.getATTRS()) and using getter/setters which these don't need.

If they get more complicated they can be moved to events, but think of ATTRS as
public API and these are not part of the public API.

https://codereview.appspot.com/7796047/diff/1/app/subapps/browser/views/view....
app/subapps/browser/views/view.js:105: 'apiHost':
window.juju_config.charmworldURL
On 2013/03/25 18:31:52, j.c.sackett wrote:
> Should this be altered so a store can be passed in, and the set is only called
> if one isn't? That seems to be the more consistent approach.

The trouble with passing a store in is that the caller has to know about things
like the juju_config. This creates a sane default for production use and testing
can replace it with another store via the ATTRS later if required. Initializer
doesn't perform any action with the store so it's safe to override after a dummy
one is created.

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

https://codereview.appspot.com/7796047/diff/1/test/test_browser_app.js#newcode29
test/test_browser_app.js:29: charmworldURL: 'http://localhost'
On 2013/03/25 18:33:16, jeff.pihach wrote:
> Maybe just make a quick note as to why you're doing this.

Added a comment about denying the store from calling out anywhere.

[rharding, 2013-03-25 19:29:56]

Please take a look.

[j.c.sackett, 2013-03-25 19:31:09]

On 2013/03/25 19:29:56, rharding wrote:
> Please take a look.

LGTM, thanks for answering my earlier concerns.

[rharding, 2013-03-25 20:00:41]

*** Submitted:

Add binding ui to switch fullscreen v sidebar.

- Update the CSS so fullsreen shows something like fullscreen
- Update the link to toggle in the search widget
- Update the search widget to take the link to use for the toggle since we
can't fire it programatically
- Start process of building a common View class for the sidebar/fullscreen to
extend. Will clean up more as we add more functionality.

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