Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

feat(typeahead): handles min-length of 0 #3600

Closed
wants to merge 2 commits into from
Closed

feat(typeahead): handles min-length of 0 #3600

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f1539ad
feat(typeahead): handles min-length of 0
zacronos Apr 30, 2015
95334ba
feat(typeahead): handles min-length of 0 (variable cleanup)
zacronos Apr 30, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/typeahead/test/typeahead.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -842,4 +842,13 @@ describe('typeahead tests', function () {
expect($scope.select_count).toEqual(1);
});

describe('minLength set to 0', function () {
it('should open typeahead if input is changed to empty string if defined threshold is 0', function () {
var element = prepareInputEl('<div><input ng-model="result" typeahead="item for item in source | filter:$viewValue" typeahead-min-length="0"></div>');
changeInputValueTo(element, '');

expect(element).toBeOpenWithActive(3, 0);
});
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it only allow empty typeahead to open, if the input is focused?

Otherwise several typeahead directives with empty inputs would simultaneously open at the start, which is probably not intended.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is the current behavior -- see the test starting on line 563, which states an element must be focused for the typeahead to be open.

Also, I believe even on focus the typeahead will not immediately open, regardless of the limit and current input length. The typeahead only opens when the input text changes.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch - you are right!

About immediate opening - this can be useful, for instance, to show most recent searches as soon as the user clicks or taps in the field. Would be a better UX than typing, in my view. That would seem to me the most intuitive behaviour for minLength = 0. Just my view, of course.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually agree with that -- especially for minLength = 0, but really any time, if the user has no idea what a valid input is, it could be a little clunky for them to have to start to type, then delete, in order to see all options. This would be a nice new feature to add -- an option to open on focus instead of on input change.


});
7 changes: 5 additions & 2 deletions src/typeahead/typeahead.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,10 @@ angular.module('ui.bootstrap.typeahead', ['ui.bootstrap.position', 'ui.bootstrap
//SUPPORTED ATTRIBUTES (OPTIONS)

//minimal no of characters that needs to be entered before typeahead kicks-in
var minSearch = originalScope.$eval(attrs.typeaheadMinLength) || 1;
var minSearch = originalScope.$eval(attrs.typeaheadMinLength);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe renaming to minLength would make it easier to read - similar to the attribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmitriz, I could change that if it is demanded, however note that I did not change that variable name -- minSearch was the name used in the original code.

Cleaning up pre-existing issues in the original code seems a rather high burden to place on a 5-line PR (not counting specs) from 1.5 years ago (this is a re-PR, the original was posted in Jan of 2014).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it was used only used in 2 places, now would be 5.
Would clean up a bit but no worries.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note that one community members' notes and reviews aren't 'demands' on how you should do things but rather suggestions/tips. There should always be an open discussion 👍

This email was sent from my iPhone and therefore subject to typos and other inaccuracies.

On 30 apr. 2015, at 02:02, Joe Ibershoff [email protected] wrote:

In src/typeahead/typeahead.js:

@@ -41,7 +41,10 @@ angular.module('ui.bootstrap.typeahead', ['ui.bootstrap.position', 'ui.bootstrap
//SUPPORTED ATTRIBUTES (OPTIONS)

   //minimal no of characters that needs to be entered before typeahead kicks-in

  •  var minSearch = originalScope.$eval(attrs.typeaheadMinLength) || 1;

  •  var minSearch = originalScope.$eval(attrs.typeaheadMinLength);
    @dmitriz, I could change that if it is demanded, however note that I did not change that variable name -- minSearch was the name used in the original code.

Cleaning up pre-existing issues in the original code seems a rather high burden to place on a 5-line PR (not counting specs) from 1.5 years ago (this is a re-PR, the original was posted in Jan of 2014).

—
Reply to this email directly or view it on GitHub.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmitriz, fair enough, you're right that this variable isn't really getting used elsewhere. I'll add that in.

if (!minSearch && minSearch !== 0) {
minSearch = 1;
}

//minimal wait time after last character typed before typeahead kicks-in
var waitTime = originalScope.$eval(attrs.typeaheadWaitMs) || 0;
Expand Down Expand Up @@ -193,7 +196,7 @@ angular.module('ui.bootstrap.typeahead', ['ui.bootstrap.position', 'ui.bootstrap

hasFocus = true;

if (inputValue && inputValue.length >= minSearch) {
if (minSearch === 0 || inputValue && inputValue.length >= minSearch) {
if (waitTime > 0) {
cancelPreviousTimeout();
scheduleSearchWithTimeout(inputValue);
Expand Down