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

Revision history for this message
Richard Harding (rharding) wrote :

Review comments

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#newcode542
app/subapps/browser/browser.js:542: extraCfg.deploy =
this.get('deployBundle');
Only set one deploy method on the details view. They can share the same
code for this. Just the deploy method needs to be different.

https://codereview.appspot.com/18920045/diff/20001/app/subapps/browser/browser.js#newcode708
app/subapps/browser/browser.js:708: extraCfg.deploy =
this.get('deploy');
pass the deploy methods into the sidebar/fullscreen views so that they
can use quicksearch to perform a deploy (vs the typical path from
details view or drag/drop)

https://codereview.appspot.com/18920045/diff/20001/app/subapps/browser/browser.js#newcode800
app/subapps/browser/browser.js:800: deployBundle:
this.get('deployBundle')
See above

https://codereview.appspot.com/18920045/diff/20001/app/subapps/browser/browser.js#newcode1165
app/subapps/browser/browser.js:1165: deployBundle: {},
document all the things!

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'));
so this changes to just deploy vs a diff method

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: );
no idea why there were two different "if search" checks. Block them into
one.

https://codereview.appspot.com/18920045/diff/20001/app/subapps/browser/views/view.js#newcode101
app/subapps/browser/views/view.js:101: this.search.on(
this is the new event the search-widget grew to handle the deploy
selection.

https://codereview.appspot.com/18920045/diff/20001/app/subapps/browser/views/view.js#newcode140
app/subapps/browser/views/view.js:140: _deployEntity: function(ev) {
mostly just sharing what the details views for bundles/charms do.

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#newcode4
app/templates/bundle-token.handlebars:4: {{#if deployButton}}
the token only displays this when requested. The search widget passes
this in for use and other token uses don't care.

https://codereview.appspot.com/18920045/diff/20001/app/templates/bundle-token.handlebars#newcode5
app/templates/bundle-token.handlebars:5: {{#ifFlag 'searchDeploy'}}
we only feature flag the UX since it's a UX driven feature. Simpler than
FF'ing all the code.

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}}
Document the real usage.

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#newcode212
app/widgets/charm-search.js:212: deployButton:
isCategory(charm.get('id')) ? false : true
note that we don't set the deployButton for the category options in the
result set.

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 is the giant hack to make this all work. Clicking anything in here
is touch because it fires a AC "itemSelect" event regardless of where
you click. The only way to NOT fire an itemSelect on here is to override
the original method and do the check/work here.

https://codereview.appspot.com/18920045/diff/20001/app/widgets/charm-search.js#newcode330
app/widgets/charm-search.js:330: } else {
this is what the original method does. So had to dupe these 3 lines from
YUI itself.

https://codereview.appspot.com/18920045/diff/20001/app/widgets/charm-search.js#newcode372
app/widgets/charm-search.js:372: if (ev.result.raw.charm) {
this is a drive-by to make selecting a quicksearch result for a bundle
to work. We have to generate the 'bundle' charmId to use.

https://codereview.appspot.com/18920045/diff/20001/lib/views/browser/bws-searchbox.less
File lib/views/browser/bws-searchbox.less (right):

https://codereview.appspot.com/18920045/diff/20001/lib/views/browser/bws-searchbox.less#newcode69
lib/views/browser/bws-searchbox.less:69: .token.tiny {
drive by fixing some alignment of the text in the quicksearch results so
they're centered better.

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

https://codereview.appspot.com/18920045/diff/20001/test/test_browser_search_widget.js#newcode209
test/test_browser_search_widget.js:209: cleanIconHelper =
utils.stubCharmIconPath();
this can go away. Left over from debugging an issue.

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

« Back to merge proposal