Code review comment for lp:~rharding/juju-gui/deploy-quicksearch

Thanks for this feature! I'll be doing the QA now, just wanted to get my
comments landed so we can discuss.

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

https://codereview.appspot.com/18920045/diff/20001/app/subapps/browser/browser.js#newcode541
app/subapps/browser/browser.js:541: if (entityId.indexOf('bundle') !==
-1) {
Because everywhere else that uses 'deploy' is using it for only charms
and 'deployBundle' for bundles, I feel that this adds in a level of
complexity that isn't easy to follow. I don't see any benefit to this
change.

https://codereview.appspot.com/18920045/diff/20001/app/subapps/browser/browser.js#newcode1165
app/subapps/browser/browser.js:1165: deployBundle: {},
Thanks for adding these in!

https://codereview.appspot.com/18920045/diff/20001/app/widgets/charm-search.js
File app/widgets/charm-search.js (right):

https://codereview.appspot.com/18920045/diff/20001/app/widgets/charm-search.js#newcode273
app/widgets/charm-search.js:273: this.ac._onItemClick = function(ev) {
We should subclass the ac widget vs doing this monkeypatch. otherwise it
will be difficult to debug if the api changes down the road when we
upgrade YUI.

https://codereview.appspot.com/18920045/diff/20001/app/widgets/charm-search.js#newcode327
app/widgets/charm-search.js:327: this.hide();
You have probably looked into this already, but just in case...Does this
shut down any pending ac requests to avoid the result list from popping
up in the future? I'm just a little concerned that we are hiding the
list element but keeping the ac functional in the background since we
are short circuiting the real code.

https://codereview.appspot.com/18920045/diff/20001/app/widgets/charm-search.js#newcode331
app/widgets/charm-search.js:331: var itemNode = ev.currentTarget;
Instead of simply copying these methods a better approach would simply
be:

this._ac.prototype._onItemClick(ev);

or if you subclass it (I think):

MyNewAutocomplete.superclass.constructor._onItemClick(ev).bind(this);

This way any changes to the parent method won't require us to update
this part of the conditional.

https://codereview.appspot.com/18920045/diff/20001/app/widgets/charm-search.js#newcode373
app/widgets/charm-search.js:373: id = ev.result.raw.charm.id;
Thanks for this fix!

https://codereview.appspot.com/18920045/diff/20001/test/test_browser_app.js
File test/test_browser_app.js (right):

https://codereview.appspot.com/18920045/diff/20001/test/test_browser_app.js#newcode182
test/test_browser_app.js:182: fakeStore = new
Y.juju.charmworld.APIv2({});
Shouldn't we also have tests here(and below) for the v3 api?

https://codereview.appspot.com/18920045/