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

LGTM with comments; thank you!

Gary

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#newcode1165
app/subapps/browser/browser.js:1165: deployBundle: {},
On 2013/10/29 18:31:54, rharding wrote:
> document all the things!

:-) cool

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

https://codereview.appspot.com/18920045/diff/20001/app/subapps/browser/views/bundle.js#newcode76
app/subapps/browser/views/bundle.js:76:
this.get('deploy')(bundle.get('data'));
On 2013/10/29 18:31:54, rharding wrote:
> so this changes to just deploy vs a diff method

The only downside to the new approach is that sometimes we separate the
two names (deploy vs deployBundle) and sometimes we merge them. That
makes it just a bit harder to figure out what's going on while reading.
In your shoes, I'd be tempted to rename the charm-only deploy function
to "deployCharm" so as to make a parallel with "deployBundle" and a
contrast with the more mutable "deploy". I am sure there are other
solutions, some better than this one. Moreover, this is just a
suggestion. What you have done is OK with me as is, and an improvement.

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

https://codereview.appspot.com/18920045/diff/20001/app/subapps/browser/views/view.js#newcode93
app/subapps/browser/views/view.js:93: );
On 2013/10/29 18:31:54, rharding wrote:
> no idea why there were two different "if search" checks. Block them
into one.

Heh. cool.

https://codereview.appspot.com/18920045/diff/20001/app/subapps/browser/views/view.js#newcode318
app/subapps/browser/views/view.js:318: deploy: {},
So this is the one I'd call deployCharm.

https://codereview.appspot.com/18920045/diff/20001/app/templates/bundle-token.handlebars
File app/templates/bundle-token.handlebars (right):

https://codereview.appspot.com/18920045/diff/20001/app/templates/bundle-token.handlebars#newcode5
app/templates/bundle-token.handlebars:5: {{#ifFlag 'searchDeploy'}}
On 2013/10/29 18:31:54, rharding wrote:
> we only feature flag the UX since it's a UX driven feature. Simpler
than FF'ing
> all the code.

Cool.

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

https://codereview.appspot.com/18920045/diff/20001/app/views/utils.js#newcode1567
app/views/utils.js:1567: * {{#ifFlag 'flag_name'}} {{/ifFlag}}
On 2013/10/29 18:31:54, rharding wrote:
> Document the real usage.

Uh-oh, the doc string is becoming dangerously close to being helpful.
;-)

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) {
This comment, or a variation of it, deserves to be in the code, IMO.

https://codereview.appspot.com/18920045/diff/20001/app/widgets/charm-search.js#newcode275
app/widgets/charm-search.js:275: // ignore the way autocomplete works.
It's more of a 'quick search'
'Course, I think we agreed that this is not really autocomplete after
all, anyway.

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')) {
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.

https://codereview.appspot.com/18920045/diff/20001/app/widgets/charm-search.js#newcode377
app/widgets/charm-search.js:377: // with entity? Something to clean up.
Entity seems simplest and reasonable.

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({});
v2? expected v3, but doubt it matters.

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