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.
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 charm.js: 56: this.on('load', function() { this.loaded = true;
app/models/
});
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 charm.js: 84: options.get_charm(
app/models/
Why the two naming styles on these_twoMethods? get_charm, loadByPath?
https:/ /codereview. appspot. com/6733067/ diff/1/ app/templates/ charm-search- result. handlebars charm-search- result. handlebars (right):
File app/templates/
https:/ /codereview. appspot. com/6733067/ diff/1/ app/templates/ charm-search- result. handlebars# newcode13 charm-search- result. handlebars: 13: {{#if {{owner} }/{{/if} }{{package_ name}}< /a>
app/templates/
owner}}
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/