WIP - Bundle details view

WIP - Bundle details view

This is a WIP fist pass for the bundle details view. Because there is 
no way to access this from the application you can access it by
using the following URL:

http://localhost:8888/bundle/~bac/wiki/3/wiki/:flags:/charmworldv3/

https://code.launchpad.net/~hatch/juju-gui/bundle-detail-view/+merge/188456

(do not edit description out of merge proposal)

[jeff.pihach, 2013-09-30 21:06:39]

Please take a look.

[jeff.pihach, 2013-09-30 21:13:13]

Please take a look.

[rharding, 2013-10-01 01:06:36]

I think this is exactly on the right path. I'd tweak a few things. I think the
View can be a bit simpler by dealing with a single ATTR for a model instance or
a token id and that logic should be pushed to the store. That way all those
decisions are made there. 

We can chat about details later but looks good and thanks for working on getting
this into play.

https://codereview.appspot.com/14153043/diff/4001/app/store/charmworld.js
File app/store/charmworld.js (right):

https://codereview.appspot.com/14153043/diff/4001/app/store/charmworld.js#new...
app/store/charmworld.js:34: juju.charmworld = ns;
ns is already defined? why do we need juju.charmworld

https://codereview.appspot.com/14153043/diff/4001/app/store/charmworld.js#new...
app/store/charmworld.js:234: Public method to make an API call to fetch a bundle
from Charmworld.
don't need "Public method" we already can tell that.

https://codereview.appspot.com/14153043/diff/4001/app/store/charmworld.js#new...
app/store/charmworld.js:242: bundle: function(bundleID, callbacks, bindScope) {
if you compare this to the charm call, there's some work we could do here around
the caching. We should add support for caching details as we do in the charms so
that subsequent calls will be instantly available. The cache is maintained by
the store instance I think as I know it's shared around both Browser and main
app code.

https://codereview.appspot.com/14153043/diff/4001/app/store/charmworld.js#new...
app/store/charmworld.js:247: API call to fetch a charm's details.
bundle's details.

https://codereview.appspot.com/14153043/diff/4001/app/store/charmworld.js#new...
app/store/charmworld.js:273: promiseCharm: function(charmId, cache,
defaultSeries) {
should we have a promiseBundle? This came after the original charm call and
maybe working with a promise from the start would be nice here?

https://codereview.appspot.com/14153043/diff/4001/app/store/charmworld.js#new...
app/store/charmworld.js:383: iconpath: function(charmID) {
did you verify this works for bundle ids?

https://codereview.appspot.com/14153043/diff/4001/app/store/charmworld.js#new...
app/store/charmworld.js:415: buildCategoryIconPath: function(categoryID) {
any calls to this are hints that we need to look for the later check for using
the default charm icon and add a check for default bundle icon.

https://codereview.appspot.com/14153043/diff/4001/app/store/charmworld.js#new...
app/store/charmworld.js:509: related: function(charmID, callbacks, bindScope) {
we'll need a bundle related method as well. Per the call with Gary today,
related bundles are lists of other bundles in the same basket as the current
one.

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

https://codereview.appspot.com/14153043/diff/4001/app/subapps/browser/browser...
app/subapps/browser/browser.js:924: if (idBits.length > 1) {
I'm assuming existing tests pass with this change?

https://codereview.appspot.com/14153043/diff/4001/app/subapps/browser/views/c...
File app/subapps/browser/views/charm.js (right):

https://codereview.appspot.com/14153043/diff/4001/app/subapps/browser/views/c...
app/subapps/browser/views/charm.js:36: ns.BrowserCharmView =
Y.Base.create('browser-view-charmview', Y.View, [
I'd change this to BrowserTokenView

https://codereview.appspot.com/14153043/diff/4001/app/subapps/browser/views/c...
app/subapps/browser/views/charm.js:762: _renderBundleView: function(bundle) {
this seems to be missing a bunch of shared logic with _renderCharmView. The
check for use in the inspector, are there any tabs? What about sharing
icon/setup?

https://codereview.appspot.com/14153043/diff/4001/app/subapps/browser/views/c...
app/subapps/browser/views/charm.js:781: charmID = this.get('charmID'),
so again, maybe the tokenID is the new thing?

https://codereview.appspot.com/14153043/diff/4001/app/subapps/browser/views/c...
app/subapps/browser/views/charm.js:786: if (this.get('charm')) {
rather than having diff charm/bundle attrs what about var token =
this.get('token'); if (token.isCharm()) else if (token.isBundle()) ?

Or even adding a thing to the model to have a type. 

this.call('_render' + token.type + 'View') or something.

https://codereview.appspot.com/14153043/diff/4001/app/subapps/browser/views/c...
app/subapps/browser/views/charm.js:793: if (charmID.indexOf('bundle') === -1) {
I feel like this check should be in the store vs here in the view. Maybe the
cleanest thing is to combine .charm/.bundle into a method where you pass an id
and the store does the logic. Checking for 'bundle' in the id seems to be an
implementation detail of the store and not something I'd have Views worry about.

https://codereview.appspot.com/14153043/diff/4001/app/templates/bundle.handle...
File app/templates/bundle.handlebars (right):

https://codereview.appspot.com/14153043/diff/4001/app/templates/bundle.handle...
app/templates/bundle.handlebars:7: <img src="{{storeId}}" alt="{{ name }} icon"
class="icon">
this needs to exist for bundles. They'll have icons as well.

[jeff.pihach, 2013-10-01 02:14:58]

Thanks for the review. Replies are below - lets chat in the morning and then I
will start a few real branches to tackle these changes.

https://codereview.appspot.com/14153043/diff/4001/app/store/charmworld.js
File app/store/charmworld.js (right):

https://codereview.appspot.com/14153043/diff/4001/app/store/charmworld.js#new...
app/store/charmworld.js:34: juju.charmworld = ns;
Not sure, this was from the old code.

https://codereview.appspot.com/14153043/diff/4001/app/store/charmworld.js#new...
app/store/charmworld.js:242: bundle: function(bundleID, callbacks, bindScope) {
Right, I wasn't sure how the cache was implemented so I left it out at this
point to get a functional example asap.

https://codereview.appspot.com/14153043/diff/4001/app/store/charmworld.js#new...
app/store/charmworld.js:273: promiseCharm: function(charmId, cache,
defaultSeries) {
I agree, as I was reading through this stuff it would be great if we only used
the promises for charms and bundles.

https://codereview.appspot.com/14153043/diff/4001/app/store/charmworld.js#new...
app/store/charmworld.js:383: iconpath: function(charmID) {
It does not so I removed it from the template - atm it looks like we only have
default icons, but will confirm in the am.

https://codereview.appspot.com/14153043/diff/4001/app/store/charmworld.js#new...
app/store/charmworld.js:509: related: function(charmID, callbacks, bindScope) {
Hmm good call - that can be a follow-up

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

https://codereview.appspot.com/14153043/diff/4001/app/subapps/browser/browser...
app/subapps/browser/browser.js:924: if (idBits.length > 1) {
You bet - I haven't manually checked the validity of all the paths yet but all
of the tests pass and qa was ok.

https://codereview.appspot.com/14153043/diff/4001/app/subapps/browser/views/c...
File app/subapps/browser/views/charm.js (right):

https://codereview.appspot.com/14153043/diff/4001/app/subapps/browser/views/c...
app/subapps/browser/views/charm.js:36: ns.BrowserCharmView =
Y.Base.create('browser-view-charmview', Y.View, [
We were trying to come up with a good name :)

https://codereview.appspot.com/14153043/diff/4001/app/subapps/browser/views/c...
app/subapps/browser/views/charm.js:762: _renderBundleView: function(bundle) {
It's definitely missing a lot of logic :) It's still a WIP - but we also don't
have UX for this view yet so any effort spent here might be wasted - we can talk
in the am.

https://codereview.appspot.com/14153043/diff/4001/app/subapps/browser/views/c...
app/subapps/browser/views/charm.js:781: charmID = this.get('charmID'),
Yeah I had intended on renaming this assuming the impl made sense.

https://codereview.appspot.com/14153043/diff/4001/app/subapps/browser/views/c...
app/subapps/browser/views/charm.js:786: if (this.get('charm')) {
I was also thinking of something similar but I didn't want to modify that deep
for this prototype

https://codereview.appspot.com/14153043/diff/4001/app/subapps/browser/views/c...
app/subapps/browser/views/charm.js:793: if (charmID.indexOf('bundle') === -1) {
I think that this only belongs in the store if we move to a get('token') as
mentioned above because we will still need to handle different success paths.

https://codereview.appspot.com/14153043/diff/4001/app/templates/bundle.handle...
File app/templates/bundle.handlebars (right):

https://codereview.appspot.com/14153043/diff/4001/app/templates/bundle.handle...
app/templates/bundle.handlebars:7: <img src="{{storeId}}" alt="{{ name }} icon"
class="icon">
Ahh ok. The API call didn't have a key for icon so I assumed that they only used
the default.