Implement the browser search control widget.

Implement the browser search control widget.

- Add a new widget containing the search input
- Provide events to the outside world to update/watch for search phrase
changes
- Add tests
- Hook widget into the render of the fullscreen and sidebar views.

https://code.launchpad.net/~rharding/juju-gui/search-widget/+merge/152737

(do not edit description out of merge proposal)

[rharding, 2013-03-12 15:30:52]

Please take a look.

[jeff.pihach, 2013-03-12 16:25:35]

Looks good! Just a few small comments/changes below

https://codereview.appspot.com/7519044/diff/1/app/subapps/browser/views/fulls...
File app/subapps/browser/views/fullscreen.js (right):

https://codereview.appspot.com/7519044/diff/1/app/subapps/browser/views/fulls...
app/subapps/browser/views/fullscreen.js:45:
search.render(tpl_node.one('.bws-search'));
At this point you are rendering a widget into an element which isn't yet
rendered onto the page. What you might notice is that you will end up with odd
bugs when doing this because of dom calculations (scrollview has this issue).

A workaround is to put these in an 'afterRender' method and then call that
method in the showView() callback.

You don't need to change this - I was just pointing that out in the event you
run into that issue :-)

https://codereview.appspot.com/7519044/diff/1/app/subapps/browser/views/sideb...
File app/subapps/browser/views/sidebar.js (right):

https://codereview.appspot.com/7519044/diff/1/app/subapps/browser/views/sideb...
app/subapps/browser/views/sidebar.js:42: _bindEvents: function() {
Where is this method called from?

https://codereview.appspot.com/7519044/diff/1/app/subapps/browser/views/sideb...
app/subapps/browser/views/sidebar.js:44:
this.search.on(this.search.EVT_UPDATE_SEARCH, this._searchChanged, this);
These events aren't being detached on destroy

https://codereview.appspot.com/7519044/diff/1/app/subapps/browser/views/sideb...
app/subapps/browser/views/sidebar.js:103:
this.search.render(tpl_node.one('.bws-search'));
See the rendering outside of the DOM issue comment from the previous file.

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

https://codereview.appspot.com/7519044/diff/1/app/widgets/charm-search.js#new...
app/widgets/charm-search.js:110:
this.get('contentBox').one('input').set('value', '');
If the input is in focus then this will trigger valueChange, I recommend:

input.blur();
input.set('value', '');

https://codereview.appspot.com/7519044/diff/1/app/widgets/charm-search.js#new...
app/widgets/charm-search.js:175:
this.get('boundingBox').one('input').set('value', newval);
If this input is in focus then it will trigger valueChange. See above.

https://codereview.appspot.com/7519044/diff/1/test/test_browser_search_widget.js
File test/test_browser_search_widget.js (right):

https://codereview.appspot.com/7519044/diff/1/test/test_browser_search_widget...
test/test_browser_search_widget.js:33: it('should supports search changed
events', function(done) {
s/supports/support
s/changed/change
;-)

https://codereview.appspot.com/7519044/diff/1/test/test_browser_search_widget...
test/test_browser_search_widget.js:55: it('should supports setting search
string', function() {
%s/supports/support 
:-D

[rharding, 2013-03-12 17:18:40]

Comments submitted and code updated.

https://codereview.appspot.com/7519044/diff/1/app/subapps/browser/views/fulls...
File app/subapps/browser/views/fullscreen.js (right):

https://codereview.appspot.com/7519044/diff/1/app/subapps/browser/views/fulls...
app/subapps/browser/views/fullscreen.js:45:
search.render(tpl_node.one('.bws-search'));
On 2013/03/12 16:25:35, jeff.pihach wrote:
> At this point you are rendering a widget into an element which isn't yet
> rendered onto the page. What you might notice is that you will end up with odd
> bugs when doing this because of dom calculations (scrollview has this issue).
> 
> A workaround is to put these in an 'afterRender' method and then call that
> method in the showView() callback.
> 
> You don't need to change this - I was just pointing that out in the event you
> run into that issue :-)

Thanks for the heads up. In this case I don't think I'll hit any issues with
that. I'd rather make sure that the UI rendered in one swoop vs a follow up.

https://codereview.appspot.com/7519044/diff/1/app/subapps/browser/views/sideb...
File app/subapps/browser/views/sidebar.js (right):

https://codereview.appspot.com/7519044/diff/1/app/subapps/browser/views/sideb...
app/subapps/browser/views/sidebar.js:42: _bindEvents: function() {
On 2013/03/12 16:25:35, jeff.pihach wrote:
> Where is this method called from?

Ummm...well not i's called from the end of render() :)

https://codereview.appspot.com/7519044/diff/1/app/subapps/browser/views/sideb...
app/subapps/browser/views/sidebar.js:44:
this.search.on(this.search.EVT_UPDATE_SEARCH, this._searchChanged, this);
On 2013/03/12 16:25:35, jeff.pihach wrote:
> These events aren't being detached on destroy

Updated

https://codereview.appspot.com/7519044/diff/1/app/subapps/browser/views/sideb...
app/subapps/browser/views/sidebar.js:103:
this.search.render(tpl_node.one('.bws-search'));
On 2013/03/12 16:25:35, jeff.pihach wrote:
> See the rendering outside of the DOM issue comment from the previous file.

rgr, thanks for the heads up.

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

https://codereview.appspot.com/7519044/diff/1/app/widgets/charm-search.js#new...
app/widgets/charm-search.js:110:
this.get('contentBox').one('input').set('value', '');
On 2013/03/12 16:25:35, jeff.pihach wrote:
> If the input is in focus then this will trigger valueChange, I recommend:
> 
> input.blur();
> input.set('value', '');

Thanks for bringing this up. I think the goal is to fire the event so I've added
focus() to the input. The use case for this is something where something in the
UI forces a search/filter update to be performed. If another view/widget wants
to update the search string, it should be allowed, updated, and then this widget
fires a change event so that the view can reload new data.

https://codereview.appspot.com/7519044/diff/1/app/widgets/charm-search.js#new...
app/widgets/charm-search.js:175:
this.get('boundingBox').one('input').set('value', newval);
On 2013/03/12 16:25:35, jeff.pihach wrote:
> If this input is in focus then it will trigger valueChange. See above.

rgr, thanks. Same as aboved.

[rharding, 2013-03-12 17:20:11]

Please take a look.

[rharding, 2013-03-12 17:31:55]

Please take a look.

[jeff.pihach, 2013-03-12 17:39:31]

LGTM Thanks!

[j.c.sackett, 2013-03-13 15:21:33]

LGTM, just one question below.

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

https://codereview.appspot.com/7519044/diff/13001/app/widgets/charm-search.js...
app/widgets/charm-search.js:165: syncUI: function() {},
Do you even need it, then?

[rharding, 2013-03-13 15:42:28]

*** Submitted:

Implement the browser search control widget.

- Add a new widget containing the search input
- Provide events to the outside world to update/watch for search phrase
changes
- Add tests
- Hook widget into the render of the fullscreen and sidebar views.

R=jeff.pihach, j.c.sackett
CC=
https://codereview.appspot.com/7519044