feat(clearOnSelected): allow users to clear the input instead of filling #244

Haroenv · 2018-08-06T13:05:39Z

In some use-cases, like when you're using autocomplete to make tags on a page, or using the output in any other way than prefilling the query, you don't want the suggested value to show in the input, but would rather like it to be empty.

Before this PR it was basically impossible to have your input not filled, since it came back on blur and a few different places (see resetInputValue)

fixes #241

now there's a new option:

autocomplete('#input', { clearOnSelected: true }, /* ... */)

This makes sure the input clears after selecting, rather dan filling

In some use-cases, like when you're using autocomplete to make tags on a page, or using the output in any other way than prefilling the query, you don't want the suggested value to show in the input, but would rather like it to be empty. Before this PR it was basically impossible to have your input not filled, since it came back on blur and a few different places (see `resetInputValue` fixes #241

Haroenv · 2018-08-06T13:08:09Z

@alystair, can you check this PR? you need to use the new option, but don't need to change the value in selected anymore

coveralls · 2018-08-06T14:47:05Z

Coverage increased (+0.04%) to 88.842% when pulling 66db126 on feat/clearOnSelected into c194736 on master.

alystair · 2018-08-06T15:16:24Z

Sorry being a bit of a novice here, I fetched your PR, then loaded the scripts:

<script src="//cdn.jsdelivr.net/algoliasearch/3/algoliasearch.min.js"></script>
<script src="/autocomplete.js/index.js"></script>

Resulting in the following:

index.js:3 Uncaught ReferenceError: module is not defined at index.js:3

Which leads to autocomplete is not defined in our own code. We don't use any frameworks.

alystair · 2018-08-06T15:19:46Z

If I try loading standalone/index.js directly it does

Uncaught ReferenceError: require is not defined at index.js:4

Haroenv · 2018-08-06T15:39:06Z

@alystair for now you'll only be able to try it by cloning the repository, then you do yarn && yarn build and copy the file out of /dist in place of your existing one

Haroenv · 2018-08-06T15:42:34Z

I uploaded the built file: https://gist.github.com/Haroenv/16d566e435cbafee32be431e045651c7

You can link this directly: https://rawgit.com/Haroenv/16d566e435cbafee32be431e045651c7/raw/5efc69ba498ffb4aed68fb1619e3c1dfa1ae6a0c/autocomplete.min.js

alystair · 2018-08-06T15:58:02Z

Doesn't seem to do anything, it reacts the same way as originally shown in issue.

Here's the code on my end:

let brands = client.initIndex('brands');
autocomplete('[name=sponsors]',
	{ clearOnSelected:true },{
		source: autocomplete.sources.hits(brands, {filters:'type:sponsor OR type:broadcasting'})
		,templates: {
			suggestion: function(suggestion) { return '<span>'+suggestion._highlightResult.name.value+'<img src="static/brands/'+suggestion['type'].charAt(0).toLowerCase()+'_'+suggestion.name.replace(/\s/g, '')+'.png'+'" alt=""></span>'; }
			,empty: function emptyTemplate() { return '<div class="aa-empty">Brand not in our database - consider <a href="mailto:XXXXXXX">suggesting it</a></div>'; }
		}
	}).on('autocomplete:selected', function(event, suggestion, dataset) {
		let exists = this.parentElement.parentElement.querySelector('[data-guid="'+suggestion['guid']+'"]');
		if (exists) { //existing tag
		// our tagging code
		} else { // create new tag
		// our tagging code
		}
	this.value = '';
	}).on('autocomplete:closed', function(event) {
		this.value='';
	});

Haroenv · 2018-08-06T16:04:38Z

let brands = client.initIndex('brands');
autocomplete('[name=sponsors]',
	{ clearOnSelected:true },{
		source: autocomplete.sources.hits(brands, {filters:'type:sponsor OR type:broadcasting'})
		,templates: {
			suggestion: function(suggestion) { return '<span>'+suggestion._highlightResult.name.value+'<img src="static/brands/'+suggestion['type'].charAt(0).toLowerCase()+'_'+suggestion.name.replace(/\s/g, '')+'.png'+'" alt=""></span>'; }
			,empty: function emptyTemplate() { return '<div class="aa-empty">Brand not in our database - consider <a href="mailto:XXXXXXX">suggesting it</a></div>'; }
		}
	}).on('autocomplete:selected', function(event, suggestion, dataset) {
		let exists = this.parentElement.parentElement.querySelector('[data-guid="'+suggestion['guid']+'"]');
		if (exists) { //existing tag
		// our tagging code
		} else { // create new tag
		// our tagging code
		}
	})

alystair · 2018-08-06T16:10:49Z

That doesn't change behavior on blur, as soon as it loses focus the input is restored.

I believe this is caused by this.resetInputValue(); in the _onBlur function (line 2417) - should add a check for your newly minted clearOnSelected option and we're in business. :)

Haroenv · 2018-08-06T16:32:33Z

Hmm, this didn’t happen to me when testing, but I have an idea what the cause is :)

Haroenv · 2018-08-06T16:32:58Z

src/autocomplete/typeahead.js

@@ -417,7 +418,11 @@ _.mixin(Typeahead.prototype, {
    if (typeof datum.value !== 'undefined') {
      this.input.setQuery(datum.value);
    }
-    this.input.setInputValue(datum.value, true);
+    if (this.clearOnSelected) {
+      this.input.setInputValue('', true);


Add setQuery here too

add this.input.setQuery(''); here and we gucci 👍

alystair · 2018-08-06T16:57:33Z

This made no difference, the only fix that worked for me was to comment out this.resetInputValue(); in the _onBlur function. Even though we're wiping input value (like I did by just adding this.value='' in the original suggest callback event) it resets it to the this.query value which is untouched.

alystair · 2018-08-06T17:01:01Z

if you add this.input.setQuery(''); to your latest revision it works well.

Haroenv · 2018-08-06T17:36:27Z

perfect, I had that initially but accidentally reversed it while developing the tests. It should work completely now @alystair

alystair · 2018-08-06T17:43:02Z

Great, ship it 👍

Haroenv · 2018-08-07T13:44:59Z

Demo of this behaviour in a codesandbox: https://codesandbox.io/s/j76mlmzq4v

(would really like a build being done in travis that can be hot linked :p )

bobylito

I have a few suggestions. Otherwise it seems solid 👍

bobylito · 2018-08-07T13:46:28Z

test/unit/typeahead_spec.js

+      view.input.trigger('enterKeyed', $e);
+
+      expect(spy).toHaveBeenCalled();
+      expect(view.input.setQuery).toHaveBeenCalledWith('');


Can you check the value of this.$input rather than checking that some methods have been called (in order to not test how the code is done)?
Also would it be possible to have some value to check that there is an actual change of state (this would be a requirement if we can test without spy)?

Makes sense, but didn't know how to set a query here via code 🤔

bobylito · 2018-08-07T13:47:11Z

test/unit/typeahead_spec.js

+      view.$input.on('autocomplete:selected', spy);
+      view.input.trigger('enterKeyed', $e);
+
+      expect(spy).toHaveBeenCalled();


Can you test the values that the callback receives?

That's already tested in other tests, this behaviour doesn't change that

Why test that it has been called. This feature could mess with the parameters if we do not know the actual implementation.

bobylito · 2018-08-07T13:50:53Z

src/autocomplete/typeahead.js

@@ -31,6 +31,7 @@ function Typeahead(o) {
  this.openOnFocus = !!o.openOnFocus;
  this.minLength = _.isNumber(o.minLength) ? o.minLength : 1;
  this.autoWidth = (o.autoWidth === undefined) ? true : !!o.autoWidth;
+  this.clearOnSelected = !!o.clearOnSelected;


What about using Boolean(o.clearOnSelected) instead of !!o.clearOnSelected?

this is in the same style as the other options, technically both are the same :)

They are the same, indeed. Just not used to see that anymore. If it's consistent, good for me.

bobylito

LGTM 👍

alystair · 2018-08-07T15:26:56Z

Is there an ETA of when this change will hit the CDNs? We'll use a local copy until then. Thanks again.

Haroenv · 2018-08-07T15:59:23Z

@alystair I'll release tomorrow morning

Haroenv requested a review from redox August 6, 2018 13:05

Haroenv commented Aug 6, 2018

View reviewed changes

fix: make sure both query and input are ''

4e3cd4a

chore: update test

6be5244

Haroenv requested review from samouss and bobylito August 7, 2018 13:00

bobylito reviewed Aug 7, 2018

View reviewed changes

Haroenv added 2 commits August 7, 2018 16:35

chore: test implem of callback too

68442cb

chore: more specific test

66db126

bobylito approved these changes Aug 7, 2018

View reviewed changes

Haroenv merged commit aa2edbb into master Aug 7, 2018

samouss deleted the feat/clearOnSelected branch August 7, 2018 15:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(clearOnSelected): allow users to clear the input instead of filling #244

feat(clearOnSelected): allow users to clear the input instead of filling #244

Haroenv commented Aug 6, 2018 •

edited

Loading

Haroenv commented Aug 6, 2018

coveralls commented Aug 6, 2018 •

edited

Loading

alystair commented Aug 6, 2018

alystair commented Aug 6, 2018

Haroenv commented Aug 6, 2018

Haroenv commented Aug 6, 2018

alystair commented Aug 6, 2018 •

edited

Loading

Haroenv commented Aug 6, 2018

alystair commented Aug 6, 2018

Haroenv commented Aug 6, 2018

Haroenv Aug 6, 2018

alystair Aug 6, 2018

alystair commented Aug 6, 2018

alystair commented Aug 6, 2018

Haroenv commented Aug 6, 2018

alystair commented Aug 6, 2018

Haroenv commented Aug 7, 2018

bobylito left a comment

bobylito Aug 7, 2018

Haroenv Aug 7, 2018

bobylito Aug 7, 2018

Haroenv Aug 7, 2018

bobylito Aug 7, 2018

bobylito Aug 7, 2018

Haroenv Aug 7, 2018

bobylito Aug 7, 2018

bobylito left a comment

alystair commented Aug 7, 2018 •

edited

Loading

Haroenv commented Aug 7, 2018

feat(clearOnSelected): allow users to clear the input instead of filling #244

feat(clearOnSelected): allow users to clear the input instead of filling #244

Conversation

Haroenv commented Aug 6, 2018 • edited Loading

Haroenv commented Aug 6, 2018

coveralls commented Aug 6, 2018 • edited Loading

alystair commented Aug 6, 2018

alystair commented Aug 6, 2018

Haroenv commented Aug 6, 2018

Haroenv commented Aug 6, 2018

alystair commented Aug 6, 2018 • edited Loading

Haroenv commented Aug 6, 2018

alystair commented Aug 6, 2018

Haroenv commented Aug 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alystair commented Aug 6, 2018

alystair commented Aug 6, 2018

Haroenv commented Aug 6, 2018

alystair commented Aug 6, 2018

Haroenv commented Aug 7, 2018

bobylito left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobylito left a comment

Choose a reason for hiding this comment

alystair commented Aug 7, 2018 • edited Loading

Haroenv commented Aug 7, 2018

Haroenv commented Aug 6, 2018 •

edited

Loading

coveralls commented Aug 6, 2018 •

edited

Loading

alystair commented Aug 6, 2018 •

edited

Loading

alystair commented Aug 7, 2018 •

edited

Loading