Refactored bundle view to make it easier to test

Refactored bundle view to make it easier to test



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

(do not edit description out of merge proposal)

[jeff.pihach, 2013-10-07 18:14:20]

Please take a look.

[rharding, 2013-10-07 18:21:21]

LGTM with comments. I think the "Do I have an entity or not" is still buried a
bit vs visible right in render. Aside from that I think this reads nicer and
will be easier to test.

https://codereview.appspot.com/14454050/diff/1/app/subapps/browser/views/bund...
File app/subapps/browser/views/bundle.js (right):

https://codereview.appspot.com/14454050/diff/1/app/subapps/browser/views/bund...
app/subapps/browser/views/bundle.js:67: return new Y.Promise(function(resolve,
reject) {
can we just skip the promise altogether if we have the entity?

https://codereview.appspot.com/14454050/diff/1/app/subapps/browser/views/bund...
app/subapps/browser/views/bundle.js:96: import: bundle.get('data')
this is great. Exactly what I mean. The data section of a bundle doesn't have
nearly enough info to be useful for showing information about charms down the
road.

https://codereview.appspot.com/14454050/diff/1/app/subapps/browser/views/bund...
app/subapps/browser/views/bundle.js:135: }, {size: [480, 360]}));
can we call this size out to a constant or something more obvious to help
catch/update in the future? Or maybe ATTRS.

https://codereview.appspot.com/14454050/diff/1/app/subapps/browser/views/bund...
app/subapps/browser/views/bundle.js:151: this._fetchData().
this is where I mean you could just: 

if (this.get('entity')) { this._renderBundleView(this.get('entity')); 

else do the work with the promise/loading.

I do think this render call reads nicely.

[jeff.pihach, 2013-10-07 18:25:19]

Thanks for the review!

https://codereview.appspot.com/14454050/diff/1/app/subapps/browser/views/bund...
File app/subapps/browser/views/bundle.js (right):

https://codereview.appspot.com/14454050/diff/1/app/subapps/browser/views/bund...
app/subapps/browser/views/bundle.js:67: return new Y.Promise(function(resolve,
reject) {
See comment below

https://codereview.appspot.com/14454050/diff/1/app/subapps/browser/views/bund...
app/subapps/browser/views/bundle.js:135: }, {size: [480, 360]}));
it was an options var before, I can swap it back.

https://codereview.appspot.com/14454050/diff/1/app/subapps/browser/views/bund...
app/subapps/browser/views/bundle.js:151: this._fetchData().
We could but I think having the promises deal with it makes the code read
cleaner and the resulting interactions cleaner.

[jeff.pihach, 2013-10-07 18:32:49]

*** Submitted:

Refactored bundle view to make it easier to test

R=rharding
CC=
https://codereview.appspot.com/14454050