Build the base of the fullscreen charm view.

Build the base of the fullscreen charm view.

- INCOMPLETE: the code will be refactored into it's own view and tests on that
view
- This fixes some bugs with the browser tabview that allows it to be used.
- Hooks up new routes for firing into the Fullscreen view and tests that we
can determine what part of the UX to focus based on route.
- Add some helpers around this shared view use.
- Adds a template for the charm details view and wires up the tabview.
- Hooks up the .charm store api call to fetch a charms data
- Wires the charm-small to allow clicking on charms to load their details
view.

https://code.launchpad.net/~rharding/juju-gui/fullscreen_charmview/+merge/155595

(do not edit description out of merge proposal)

[rharding, 2013-03-26 21:15:32]

Please take a look.

[jeff.pihach, 2013-03-26 21:33:10]

Thanks for this - it's starting to come together.

Some instances in the tests need to be destroyed.

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

https://codereview.appspot.com/7581045/diff/1/app/store/charm.js#newcode191
app/store/charm.js:191: * @param {Object} bindScopr the scope of *this* in the
callbacks.
typo 'bindScopr'

https://codereview.appspot.com/7581045/diff/1/app/subapps/browser/views/fulls...
File app/subapps/browser/views/fullscreen.js (right):

https://codereview.appspot.com/7581045/diff/1/app/subapps/browser/views/fulls...
app/subapps/browser/views/fullscreen.js:72: var charm = new
models.BrowserCharm(data),
How is this instance cleaned up?

https://codereview.appspot.com/7581045/diff/1/test/test_tabview.js
File test/test_tabview.js (right):

https://codereview.appspot.com/7581045/diff/1/test/test_tabview.js#newcode20
test/test_tabview.js:20: tabview = new Y.juju.widgets.browser.TabView();
You are creating a new tabview every test but not destroying it.

https://codereview.appspot.com/7581045/diff/1/test/test_tabview.js#newcode39
test/test_tabview.js:39: tabview = new Y.juju.widgets.browser.TabView({vertical:
true});
not destroyed.

[rharding, 2013-03-27 13:11:43]

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

https://codereview.appspot.com/7581045/diff/1/app/store/charm.js#newcode191
app/store/charm.js:191: * @param {Object} bindScopr the scope of *this* in the
callbacks.
On 2013/03/26 21:33:10, jeff.pihach wrote:
> typo 'bindScopr'

doh

https://codereview.appspot.com/7581045/diff/1/app/subapps/browser/views/fulls...
File app/subapps/browser/views/fullscreen.js (right):

https://codereview.appspot.com/7581045/diff/1/app/subapps/browser/views/fulls...
app/subapps/browser/views/fullscreen.js:72: var charm = new
models.BrowserCharm(data),
On 2013/03/26 21:33:10, jeff.pihach wrote:
> How is this instance cleaned up?

When this is pulled out into it's own view it'll be an ATTR of the new view I
believe. I've added a comment with a @todo to make sure it's tied to the
instance of the new view.

https://codereview.appspot.com/7581045/diff/1/test/test_tabview.js
File test/test_tabview.js (right):

https://codereview.appspot.com/7581045/diff/1/test/test_tabview.js#newcode20
test/test_tabview.js:20: tabview = new Y.juju.widgets.browser.TabView();
On 2013/03/26 21:33:10, jeff.pihach wrote:
> You are creating a new tabview every test but not destroying it.

thanks, updated in the afterEach

[rharding, 2013-03-27 13:13:07]

Please take a look.

[jeff.pihach, 2013-03-27 14:09:58]

LGTM Thanks for making those changes!

[j.c.sackett, 2013-03-27 15:01:57]

Mostly looks good, a couple questions/comments.

https://codereview.appspot.com/7581045/diff/7001/app/subapps/browser/template...
File app/subapps/browser/templates/browser_charm.handlebars (right):

https://codereview.appspot.com/7581045/diff/7001/app/subapps/browser/template...
app/subapps/browser/templates/browser_charm.handlebars:42: </div>
I see two "changelog" classes here, but there's just the one log in the view.
Typo, or am I misunderstanding something?

https://codereview.appspot.com/7581045/diff/7001/app/widgets/browser-tabview.js
File app/widgets/browser-tabview.js (left):

https://codereview.appspot.com/7581045/diff/7001/app/widgets/browser-tabview....
app/widgets/browser-tabview.js:51: 'base',
Should base be removed? It is still using Y.Base. I realize tabview probably
brings Base with it, but explicit/implicit etc.

https://codereview.appspot.com/7581045/diff/7001/test/test_tabview.js
File test/test_tabview.js (right):

https://codereview.appspot.com/7581045/diff/7001/test/test_tabview.js#newcode12
test/test_tabview.js:12: ], function(Y) {
Did lint complain about the wrapping here, or is this personal preference? I
don't actually see any functional difference in `before`.

[rharding, 2013-03-27 15:16:46]

comments below

https://codereview.appspot.com/7581045/diff/7001/app/subapps/browser/template...
File app/subapps/browser/templates/browser_charm.handlebars (right):

https://codereview.appspot.com/7581045/diff/7001/app/subapps/browser/template...
app/subapps/browser/templates/browser_charm.handlebars:42: </div>
On 2013/03/27 15:01:57, j.c.sackett wrote:
> I see two "changelog" classes here, but there's just the one log in the view.
> Typo, or am I misunderstanding something?

Sorry, this will ahve to be worked out. By defualt there's a single 'last
changelog' entry with an option to expand which opens the second one. So there's
two using classes which are safe they'll style the same. There's just missing
bits for opening/closing/etc.

https://codereview.appspot.com/7581045/diff/7001/app/widgets/browser-tabview.js
File app/widgets/browser-tabview.js (left):

https://codereview.appspot.com/7581045/diff/7001/app/widgets/browser-tabview....
app/widgets/browser-tabview.js:51: 'base',
On 2013/03/27 15:01:57, j.c.sackett wrote:
> Should base be removed? It is still using Y.Base. I realize tabview probably
> brings Base with it, but explicit/implicit etc.

Yea, it's not needed. There's a bunch of other req's you'd have to pull in if
you wanted them all. Since you don't specifically use Y.Base in here I didn't
see a reason to keep it around.

https://codereview.appspot.com/7581045/diff/7001/test/test_tabview.js
File test/test_tabview.js (right):

https://codereview.appspot.com/7581045/diff/7001/test/test_tabview.js#newcode12
test/test_tabview.js:12: ], function(Y) {
On 2013/03/27 15:01:57, j.c.sackett wrote:
> Did lint complain about the wrapping here, or is this personal preference? I
> don't actually see any functional difference in `before`.

No, this is linter approved and was just a side effect while I was trying to
figure out the build issues. I didn't get all things reverted when I finally got
things working.

[j.c.sackett, 2013-03-27 15:23:50]

LGTM, with the addition of `base` in browser-tabview reqs per our discussion in
IRC.

Thanks for answering my questions.

[rharding, 2013-03-27 17:56:06]

*** Submitted:

Build the base of the fullscreen charm view.

- INCOMPLETE: the code will be refactored into it's own view and tests on that
view
- This fixes some bugs with the browser tabview that allows it to be used.
- Hooks up new routes for firing into the Fullscreen view and tests that we
can determine what part of the UX to focus based on route.
- Add some helpers around this shared view use.
- Adds a template for the charm details view and wires up the tabview.
- Hooks up the .charm store api call to fetch a charms data
- Wires the charm-small to allow clicking on charms to load their details
view.

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