Add showOnEmpty option to autoComplete

[FoxxMD]

I've made some small tweaks to autocomplete to provide an "openOnFocus" option. When openOnFocus is true and the user triggers a focus event on the inputTags input element the autocomplete dropdown will open automatically.

[FoxxMD]

My bad about the three test commits -- I'm fairly new to git and I was using them to make sure I was pushing to my fork...I couldn't figure out how to revert them after I'd done it.

[mbenford]

Hi @FoxxMD. Thanks for contributing! 👍

Before I can merge your PR into the master branch I'd like to ask you to do some changes:

• Delete the test commits, otherwise they'll clutter the main branch history. You can do that by rebasing your repo with the -i option. More info here.
• Write tests to cover all changes you've made. This is very important for me to accept PRs. You can quickly check what parts of the code are lacking tests by running grunt coverage (you'll notice that there are two different lines which are related to the stopImmediatePropagation() method and aren't covered by any tests; just ignore them).

I'd also like to make a couple of comments about the code, but will do that directly in the changed files.

After performing those changes, you'll need to force a push into your forked repo so the PR gets updated.

Kind regards.

[mbenford]

I don't think the minLength option should be ignored here. It has nothing to do with the openOnFocus option.

[FoxxMD]

Ignoring minLength when the flag is set insures the the menu will not close when input is blank. Perhaps I'll change the condition to (query.length < options.minLength && !options.openOnFocus) ?

[mbenford]

Oh, I get it now. But it still seems odd to me that one flag named showOnFocus interferes with another one called minLength. I mean, the way I see it, showOnFocus should only ensure the load() method is called upon the input getting focus, and nothing else. Perhaps there should be another option, say showOnEmpty that would do precisely that: call load() when the input is empty (which in the end would be the same as setting minLength to 0).

And let's not forget about issue #54, which relates to this behavior as well. There's a lot to consider here. :)

[FoxxMD]

Indeed! Another thing to think about -- addOnBlur is set to true by default. Because of this if you leave the input area you automatically get a tag made from the input. So really any time the input area gets focus the input is blank. Using what I have done still makes sense because input length will (almost) always be zero. However it does make behavior a little more opaque... I may just try and tackle #54 and refactor this into its own function.

[FoxxMD]

Thanks for the feedback! I will have everything cleaned up by tomorrow.

[FoxxMD]

I've added opening the dropdown on key down(and set up scrolling, etc.). However I am having trouble with getting my tests to work properly..

my code is as follows:

it('does prevent the down arrow keydown event from being propagated and opens dropdown', function() {

   //Arrange
   loadSuggestions(["item1"]);
   eventHandlers['input-blur']();

    // Act
    var event = sendKeyDown(KEYS.down);

    // Assert
    expect(event.isDefaultPrevented()).toBe(true);
    expect(event.isPropagationStopped()).toBe(true);
    expect(isSuggestionsBoxVisible()).toBe(true);
});

As far as I can tell the behavior of the new features work correctly (the first two assertions pass and I've stepped through in debugging to make sure everything is called correctly) but isSuggestionsBoxVisible() runs before the element has ng-hide removed from it and fails.

Any suggestions on what I can do to fix this?

Also: Would you like "open on down arrow" to be a separate pull request?

[mbenford]

In order for Angular to render the directive's markup you need to call loadSuggestions(), which is a helper function inside the main test suite. However, you shouldn't do that within that test; the "keys propagation handling" suite is meant to group tests related to keys propagation handling only. Since you've added a new option (or two, I'm not sure), for consistency's sake with other existing tests I suggest you create a new suite for each new option. Take a look at the max-results-to-show option suite for an example. You should also test if the new options are correctly initialized (i.e. their default values are correctly set).

Finally, I like the idea of having this "open on down arrow" feature as a separate PR.

[FoxxMD]

I am calling loadSuggestions()!

The keydown event triggers .load() to be called and autocomplete behaves as it should -- the problem I'm running in to is that .load() makes an async call for the queried source but the test continues to execute. The result for isSuggestionsBoxVisible() is false because the promise hasn't resolved yet in order to call .show().

I'm still new to karma/jasmine but I'm sure there must be a way to spyOn the promise in .load() and wait for it with waitsFor() before finishing the test but I have yet to get anything working. Any ideas?

I will move the keydown feature into a seperate PR and also create a full suite for each feature.

[mbenford]

Sorry. I didn't notice you were calling loadSuggestions().

In fact, I realize now that there's no need to use that function at all. All you have to do is simulate a down arrow keystroke and then assert that the loadItems function is called with an empty string. Something like the following:

it('calls the load function with an empty query when the down arrow key is pressed', function() {
    // Act
    sendKeyDown(KEYS.down)

    $timeout.flush();

    // Assert
    expect($scope.loadItems).toHaveBeenCalledWith('');
});

There are other tests ensuring that the autocomplete list is correctly rendered, so no need to do that again here. :)

[FoxxMD]

Ah I see! I was thinking of the test as having to simulate the entire action but it just needs to "cover" what hasn't been tested, that makes things so much easier!

Thanks for all the help btw. You've been very patient with me!

[FoxxMD]

I think I got everything covered! Let me know if there's anything else I need to do. thanks

[mbenford]

I guess some tweaking is still needed, but I'll do it myself. One last thing I'd like you to do is squash your commits into one, following the commit message guidelines (which I see you're already doing).

Thanks again for contributing!

[FoxxMD]

Okay I'll fix the branch up tomorrow morning.

What other tweaking needs to be done?

[mbenford]

Not much. I'm not sure if passing null is the best choice (perhaps an empty string would require less checking within the load function) and I kind of like the former name you have created (showOnFocus) because it feels more suitable to me.

But there's no point in asking you to do everything. I'm supposed to work as well. :)

[FoxxMD]

showOnEmpty felt like a saner description for the behavior than showOnFocus for a few reasons.

If it was showOnFocus

• if addOnBlur is false then on input-focus I'd have to check input for a value and pass it to .load() if it existed. This conflicts with minLength unless I want to add more checks to ensure minLength is always enforced. And at that point showOnFocus will only occur if input is empty or > minLength -- and the suggestion box opens on the next input-change anyways so it would have been a bit redundant.
• if addOnBlur is true (by default it is) then input will always be empty when input-focus is called since the input will be cleared on input-blur.

passing null instead of an empty string was also a strategic decision :)

• highlightMatchedText treats input of an empty string differently than null and was creating some weird behavior(I think its in the markdown) that output some crazy stuff. This was an issue with feat(autoComplete): Add down arrow hotkey to open suggestion box if not shown #67 but contributed to the decision.
• To add to the above I needed to check input has length on keydown from feat(autoComplete): Add down arrow hotkey to open suggestion box if not shown #67 before passing it to ensure nothing crazy happens in highlightMatchedText
• passing an empty string would require the source function to have to check that input is not null and has length (instead of just null)

Basically because of all of those things it's simpler to pass null because I have to do less checks and the user only has to worry about checking query for null instead of query.length as well.

[mbenford]

Hi @FoxxMD. Sorry for taking so long to reply your message. I'll take a closer look at your PR this week and come back to you as soon as I can.

[just-boris]

@mbenford is there any updates? I am adding auto-complete into my project and I want to use this feature

[mbenford]

@just-boris This PR is selected for the next release, v2.1.0.