yellow team mailing list archive

[Benjamin Saller <benjamin.saller@xxxxxxxxxxxxx>]

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/

-- 
https://code.launchpad.net/~gary/juju-gui/simplifycharmstore/+merge/131086
Your team Juju GUI Hackers is requested to review the proposed merge of lp:~gary/juju-gui/simplifycharmstore into lp:juju-gui.