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

Please take a look.

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#newcode278
app/widgets/charm-search.js:278: if
(ev.target.hasClass('search_add_to_canvas')) {
On 2013/10/29 19:09:46, gary.poster wrote:
> I'm guessing we can't have an event registration for
li.search_add_to_canvas or
> whatever that simply catches the event first because it is a more
specific
> selector, and then calls ev.halt()?

> If not, I suggest changing the body of this block to something like
the
> following.

> this.hide();
> this.handleDeploy(ev.target.getData());

> Then handleDeploy can look at the data and do the right thing (that
is, the code
> you have below) within its own code. That feels like a readability
improvement
> to me.

> Alternatively, you could expand this to divide up on the basis of
bundle vs
> charm.

Agreed and in Jeff's review and discussion we agreed that this version
of 'autocomplete' has gotten complex enough to be it's own module.
Three's a card to clean this up. For a start I've moved the logic into
_onDeploy(ev) and called that. It's what we'll have when moved to its
own module.

https://codereview.appspot.com/18920045/diff/20001/app/widgets/charm-search.js#newcode327
app/widgets/charm-search.js:327: this.hide();
On 2013/10/29 19:21:51, jeff.pihach wrote:
> 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.

No, that'll have to be a new feature. From what I can tell, there's not
currently request tracking/killing in AC.

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({});
On 2013/10/29 19:09:46, gary.poster wrote:
> v2? expected v3, but doubt it matters.

True, it's a fakestore never called so it didn't matter. It would make
for one fewer instance to change during the conversion to making v3 live
though so I'll update.

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