Support autocomplete of charms in search.

Support autocomplete of charms in search.

- Adds support to the charm-search widget to autocomplete as a user types
- Adds a new api endpoint to the store 'autocomplete'
- Adds the yui3-skin-sam to enable proper styling of the AC widget. This
caused some CSS changes to undo the damage in other locations.
- Update the view to only deal with the search widget when a store is present.
In tests it's not, and there's no point getting errors around it.
- Make sure we don't display metadata in the tiny charm-token sized used for
the AC results display.
- Update tests to deal with the new search widget updates.
- Add test to the new handlebars unless_eq helper. 

Testing Notes:

I know the tests around the AC widget in the search tests are a bit weak. It's
difficult to test since there's not a good way to wait for the html render to
complete with suggestions without doing timeout related code. I've tried to
test that the custom parts of the code we init are touched and working. So we
verify that our results include a charm-token html result vs a plain 'name' of
a charm. Most of the work in there is using code that's already under test,
like firing viewNavigate and such. If we find bugs I'm more than happy to
break into more complicated testing to make sure they don't reoccur, but it's
much of this is tying together code that's already under test.

https://code.launchpad.net/~rharding/juju-gui/autocomplete-widget/+merge/174016

(do not edit description out of merge proposal)

[curtis, 2013-07-10 21:39:16]

Thank for showing my your progress Rick. I have a few remarks. I look forward to
see the tests that verify the new methods work as intended.

https://codereview.appspot.com/11126043/diff/3001/app/config-debug.js
File app/config-debug.js (right):

https://codereview.appspot.com/11126043/diff/3001/app/config-debug.js#newcode34
app/config-debug.js:34: charmworldURL: 'http://staging.jujucharms.com/',
Lets revert this if we deploy the API changes to production.

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

https://codereview.appspot.com/11126043/diff/3001/app/views/utils.js#newcode1273
app/views/utils.js:1273: Y.Handlebars.registerHelper('unless_eq', function(x, y,
options) {
What? Doesn't handlebars have an "unless"? Oh, I see the if_equal above. Does
handles bars use coercion (== and !=)?

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

https://codereview.appspot.com/11126043/diff/3001/app/widgets/charm-search.js...
app/widgets/charm-search.js:137: _setupAutocomplete: function() {
I don't understand the doc above.

[rharding, 2013-07-11 04:03:03]

Replies below.

https://codereview.appspot.com/11126043/diff/3001/app/config-debug.js
File app/config-debug.js (right):

https://codereview.appspot.com/11126043/diff/3001/app/config-debug.js#newcode34
app/config-debug.js:34: charmworldURL: 'http://staging.jujucharms.com/',
On 2013/07/10 21:39:16, curtis wrote:
> Lets revert this if we deploy the API changes to production.

Definitely. It'll be gone before this is put up for review/landing.

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

https://codereview.appspot.com/11126043/diff/3001/app/views/utils.js#newcode1273
app/views/utils.js:1273: Y.Handlebars.registerHelper('unless_eq', function(x, y,
options) {
On 2013/07/10 21:39:16, curtis wrote:
> What? Doesn't handlebars have an "unless"? Oh, I see the if_equal above. Does
> handles bars use coercion (== and !=)?

This is the inverse of the if_eq I added before. They here is that you can
specify two things to compare where normally you're checking the truthy-ness of
a single variable/property. In this way I can do the checks in the
charm-token.handlebars template to conditionally show/hide things based on the
size attribute of the token and compare it directly with the 'tiny' size used to
alter rendering behavior.

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

https://codereview.appspot.com/11126043/diff/3001/app/widgets/charm-search.js...
app/widgets/charm-search.js:137: _setupAutocomplete: function() {
On 2013/07/10 21:39:16, curtis wrote:
> I don't understand the doc above.

Thanks, I'll update it. It's agree it's very shallow.

[rharding, 2013-07-11 16:08:01]

Please take a look.

[rharding, 2013-07-11 16:10:43]

Please take a look.

[jeff.pihach, 2013-07-11 18:12:01]

As far as I can tell the code is looking great but it throws a 406 when trying
to make the AC request

GET
https://manage.jujucharms.com/api/2/charms?series=precise&text=ce&type=approv...
406 (Not Acceptable)

https://codereview.appspot.com/11126043/diff/13001/app/index.html
File app/index.html (right):

https://codereview.appspot.com/11126043/diff/13001/app/index.html#newcode67
app/index.html:67: <body class="yui3-skin-sam">
I see that you copy the styles we need so we don't need this class here.

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

https://codereview.appspot.com/11126043/diff/13001/app/widgets/charm-search.j...
app/widgets/charm-search.js:116: var dataprocessor =
this.get('autocompleteDataFormatter');
I can't find where this is set to a value other than undefined

https://codereview.appspot.com/11126043/diff/13001/app/widgets/charm-search.j...
app/widgets/charm-search.js:146: inputNode:
this.get('boundingBox').one('input'),
to future proof this we may want to be more specific with this input
('input[name=bws-search]')

https://codereview.appspot.com/11126043/diff/13001/app/widgets/charm-search.j...
app/widgets/charm-search.js:169: form.one('input').set('value', ev.result.text);
This might cause the AC to trigger again.

You should blur the input first, then update the input, then focus the input as
the user will expect that the input (which the value just changed in) to be in
focus.

https://codereview.appspot.com/11126043/diff/13001/app/widgets/charm-search.j...
app/widgets/charm-search.js:225: this._setupAutocomplete();
Shouldn't we only set up the AC once per app load? Won't this create a new
instance every time the browser is opened/closed?

https://codereview.appspot.com/11126043/diff/13001/app/widgets/charm-search.j...
app/widgets/charm-search.js:319: autocompleteDataFormatter: {
I don't think this is ever set to any value.

https://codereview.appspot.com/11126043/diff/13001/lib/views/browser/bws-sear...
File lib/views/browser/bws-searchbox.less (right):

https://codereview.appspot.com/11126043/diff/13001/lib/views/browser/bws-sear...
lib/views/browser/bws-searchbox.less:43: .yui3-skin-sam .yui3-aclist-item-hover
{
remove the yui3-skin-sam as per previous comment.

[rharding, 2013-07-11 18:28:00]

Replies below, will update branch with suggestions.

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

https://codereview.appspot.com/11126043/diff/13001/app/widgets/charm-search.j...
app/widgets/charm-search.js:116: var dataprocessor =
this.get('autocompleteDataFormatter');
On 2013/07/11 18:12:01, jeff.pihach wrote:
> I can't find where this is set to a value other than undefined

app/subapps/browser/views/view.js: autocompleteDataFormatter:
this.get('store').resultsToCharmlist,

This is used to turn the API results into a ModelList we can iterate through
with full attributes without the widget gaining knowledge of models itself.

This is required since the charmToken is expecting to get a
BrowserModel.getAttrs() to be init'd and we're using that token for suggestion
display.

https://codereview.appspot.com/11126043/diff/13001/app/widgets/charm-search.j...
app/widgets/charm-search.js:146: inputNode:
this.get('boundingBox').one('input'),
On 2013/07/11 18:12:01, jeff.pihach wrote:
> to future proof this we may want to be more specific with this input
> ('input[name=bws-search]')

ok

https://codereview.appspot.com/11126043/diff/13001/app/widgets/charm-search.j...
app/widgets/charm-search.js:169: form.one('input').set('value', ev.result.text);
On 2013/07/11 18:12:01, jeff.pihach wrote:
> This might cause the AC to trigger again.
> 
> You should blur the input first, then update the input, then focus the input
as
> the user will expect that the input (which the value just changed in) to be in
> focus.

Since it's done pragmatically I don't think it fires. I'll test to make sure. If
it doesn't do it I'd hate to mess around with focus if we don't have to.

https://codereview.appspot.com/11126043/diff/13001/app/widgets/charm-search.j...
app/widgets/charm-search.js:225: this._setupAutocomplete();
On 2013/07/11 18:12:01, jeff.pihach wrote:
> Shouldn't we only set up the AC once per app load? Won't this create a new
> instance every time the browser is opened/closed?

No, the AC widget is created based on the View that's in play. It's part of the
sidebar/fullscreen and soon to be part of the minimized view. There are
differences in how things behave and the Subapp shouldn't be DOM knowledgeable.
This is working with the current way the charm-search widget is used and in
place.

https://codereview.appspot.com/11126043/diff/13001/app/widgets/charm-search.j...
app/widgets/charm-search.js:319: autocompleteDataFormatter: {
On 2013/07/11 18:12:01, jeff.pihach wrote:
> I don't think this is ever set to any value.

Answered above.

https://codereview.appspot.com/11126043/diff/13001/lib/views/browser/bws-sear...
File lib/views/browser/bws-searchbox.less (right):

https://codereview.appspot.com/11126043/diff/13001/lib/views/browser/bws-sear...
lib/views/browser/bws-searchbox.less:43: .yui3-skin-sam .yui3-aclist-item-hover
{
On 2013/07/11 18:12:01, jeff.pihach wrote:
> remove the yui3-skin-sam as per previous comment. 

k, will test.

[curtis, 2013-07-11 20:17:17]

Hi Rick. This looks good I have a question about _fetchSuggestions() tests.

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

https://codereview.appspot.com/11126043/diff/13001/app/widgets/charm-search.j...
app/widgets/charm-search.js:65: filters.text = query;
I didn't see a test that verified that filter.text was set with the query
string. I did see the test for autocomplete=true. Maybe this was implicitly
tested?

https://codereview.appspot.com/11126043/diff/13001/app/widgets/charm-search.j...
app/widgets/charm-search.js:169: form.one('input').set('value', ev.result.text);
I assume the concern here is for valuechanged (because obviously there is no
keyup). Programmatic changes are exempt from triggering a event. We need to do
something extraordinary (polling the value?) for this to become a user event.
Well I am not knowledgeable about the modern autocomplete widget. /me ponders.
Scroll to the bottom of http://yuilibrary.com/yui/docs/event/valuechange.html --
this is my understanding of the situation.

[rharding, 2013-07-12 11:30:49]

Please take a look.

https://codereview.appspot.com/11126043/diff/13001/app/index.html
File app/index.html (right):

https://codereview.appspot.com/11126043/diff/13001/app/index.html#newcode67
app/index.html:67: <body class="yui3-skin-sam">
On 2013/07/11 18:12:01, jeff.pihach wrote:
> I see that you copy the styles we need so we don't need this class here.

I do need it. The overlay for the results don't render correctly without it. I
see Brad also needs it for his work he's got in review so leaving it for now.

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

https://codereview.appspot.com/11126043/diff/13001/app/widgets/charm-search.j...
app/widgets/charm-search.js:65: filters.text = query;
On 2013/07/11 20:17:17, curtis wrote:
> I didn't see a test that verified that filter.text was set with the query
> string. I did see the test for autocomplete=true. Maybe this was implicitly
> tested?

I've added a test to this and will push it up in a moment. Thanks for catching
that this should be verified.

https://codereview.appspot.com/11126043/diff/13001/app/widgets/charm-search.j...
app/widgets/charm-search.js:169: form.one('input').set('value', ev.result.text);
On 2013/07/11 20:17:17, curtis wrote:
> I assume the concern here is for valuechanged (because obviously there is no
> keyup). Programmatic changes are exempt from triggering a event. We need to do
> something extraordinary (polling the value?) for this to become a user event.
> Well I am not knowledgeable about the modern autocomplete widget. /me ponders.
> Scroll to the bottom of http://yuilibrary.com/yui/docs/event/valuechange.html
--
> this is my understanding of the situation.  

Correct, doing this does not fire the AC widget off so I'm going to leave the
focus alone unless we come upon a bug that requires it.

[curtis, 2013-07-12 15:04:16]

Thank you rick. LGTM.

[jeff.pihach, 2013-07-12 17:27:06]

Thanks for the updates Rick, I'm going to LGTM this with the css issued being
fixed mentioned below.

latest QA showed that the tabs in the  charm details page in the browser are
improperly styled - likely picking up styles from the newly added yui3-skin-sam
class added to the body.

Also noted that the autocomplete list does not hide when clicking the
environment. This is likely caused by something in the environment event system
preventing the click from bubbling to the body where clickoutside can listen
for.

[rharding, 2013-07-24 17:29:48]

Please take a look.

[rharding, 2013-07-24 18:30:47]

*** Submitted:

Support autocomplete of charms in search.

- Adds support to the charm-search widget to autocomplete as a user types
- Adds a new api endpoint to the store 'autocomplete'
- Adds the yui3-skin-sam to enable proper styling of the AC widget. This
caused some CSS changes to undo the damage in other locations.
- Update the view to only deal with the search widget when a store is present.
In tests it's not, and there's no point getting errors around it.
- Make sure we don't display metadata in the tiny charm-token sized used for
the AC results display.
- Update tests to deal with the new search widget updates.
- Add test to the new handlebars unless_eq helper. 

Testing Notes:

I know the tests around the AC widget in the search tests are a bit weak. It's
difficult to test since there's not a good way to wait for the html render to
complete with suggestions without doing timeout related code. I've tried to
test that the custom parts of the code we init are touched and working. So we
verify that our results include a charm-token html result vs a plain 'name' of
a charm. Most of the work in there is using code that's already under test,
like firing viewNavigate and such. If we find bugs I'm more than happy to
break into more complicated testing to make sure they don't reoccur, but it's
much of this is tying together code that's already under test.

R=curtis, jeff.pihach
CC=
https://codereview.appspot.com/11126043