Code review comment for lp:~gary/juju-gui/simplifycharmstore

Revision history for this message
Benjamin Saller (bcsaller) wrote :

Even though this is a large branch I think it removes more code than it
adds which is always a plus. I had some minor feedback but this LGTM.

I'd still rather Kapil get a chance to look this over but I think the
general architectural issues that were present before (and which I
didn't take into account either) are no longer present.

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

https://codereview.appspot.com/6733067/diff/1/app/models/charm.js#newcode56
app/models/charm.js:56: this.on('load', function() { this.loaded = true;
});
Don't you need to either use self here or pass this as context to the
'on' call?

https://codereview.appspot.com/6733067/diff/1/app/models/charm.js#newcode84
app/models/charm.js:84: options.get_charm(
Why the two naming styles on these_twoMethods? get_charm, loadByPath?

https://codereview.appspot.com/6733067/diff/1/app/templates/charm-search-result.handlebars
File app/templates/charm-search-result.handlebars (right):

https://codereview.appspot.com/6733067/diff/1/app/templates/charm-search-result.handlebars#newcode13
app/templates/charm-search-result.handlebars:13: {{#if
owner}}{{owner}}/{{/if}}{{package_name}}</a>
I thought you didn't need the #if when there is no content other than
the var which can default to null, no?

https://codereview.appspot.com/6733067/

« Back to merge proposal