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

refactor(typeahead): Add typeahead controller (WIP) #888

Closed

Conversation

chrisirhc
Copy link
Contributor

Tasks left:

  • Clean up duplicated code
  • Add tests
  • See if there are any other additions needed
  • Finalize public API

Current known issues:

  • Breaks defaults. Default min length is no longer there.

Allows other directives to extend functionality of controller.

Since typeahead-popup is a sibling that is right after the typeahead
input element, the controller can't be shared through the directive's
'require' option. As such, it's shared through the new child scope.

Example Typeahead Extension Directive

Here's an example that extends the typeahead directive to open on focus (#764):
http://plnkr.co/edit/UAWfTE
(Note that the example is based on my build which is on my demo branch https://github.com/chrisirhc/bootstrap-1/tree/demo/typeahead-extension , as typeahead needs fixes #886, #887 to get that functionality)

The directive is just:

.directive('typeaheadOpenOnFocus', function () {
  return {
    require: ['typeahead', 'ngModel'],
    link: function (scope, element, attr, ctrls) {
      element.bind('focus', function () {
        ctrls[0].getMatchesAsync(ctrls[1].$viewValue);
        scope.$apply();
      });
    }
  };
})

This follows ideas of ngModel and its ngModelController in AngularJS. The typeaheadController provides an API (just another object) that can be overridden, added, or called by other directives.

@cindoum
Copy link

cindoum commented Sep 25, 2013

Is this directive working with angular-ui 0.6 + angular 1.2 ? I get
[$compile:ctreq] Controller 'typeahead', required by directive 'typeaheadOpenOnFocus', can't be found!

@bkc
Copy link

bkc commented Oct 3, 2013

I would love to see this in a future release, I really need to show the pop-up on focus when minlength=0

@trask
Copy link

trask commented Oct 19, 2013

This would be great. I tested the PR and it worked well for me. Minor issue, the select was not closing after backspacing over all text. I added (back) a call to resetMatches() to fix, see https://github.com/trask/angular-ui-bootstrap/commit/454cc150978ca1b096d72e8b04fb4baf9d3f1594. Thanks.

@chrisirhc
Copy link
Contributor Author

Just an update for this, I'm planning to look at the API for typeahead.js to see if I can get some inspiration on implementing this API.

@chrisirhc
Copy link
Contributor Author

I've given some more thought about this, and this is what I've come up with so far. @pkozlowski-opensource , please take a look at let me know what you think. I've kept this as many separate commits in order to aid code review. I can squash the commits later on, for a tidier commit history.

Right now, typeaheadController exposes the following methods:

  • setQuery
  • getMatches
  • select

queryParsers

The one API that I've added that seems to be useful for many people is the queryParsers.
queryParsers is an array similar to ngModelControlller.$parsers. However, it resolves promises returned by the parsers. As such, there's a lot more that can be done with it.

There are some private methods that are marked by a preceding underscore such as _nextMatch. This indicates that users should not rely on these methods in their directives as they are undocumented API. More variables can be made private if needed, later.

@chrisirhc
Copy link
Contributor Author

Also, @bekos , any thoughts?

@pkozlowski-opensource
Copy link
Member

@chrisirhc would be awesome if you could squash / rebase those commits. Typeahead's controller is one of the topics I would like to look into after we are done with BS3 / NG1.2 support.

@chrisirhc
Copy link
Contributor Author

@pkozlowski-opensource , I just did another pass on this. It's still looking quite untidy, but ready for comments.

@@ -29,223 +29,236 @@ angular.module('ui.bootstrap.typeahead', ['ui.bootstrap.position', 'ui.bootstrap
};
}])

.directive('typeahead', ['$compile', '$parse', '$q', '$timeout', '$document', '$position', 'typeaheadParser',
function ($compile, $parse, $q, $timeout, $document, $position, typeaheadParser) {
.controller('typeaheadController', [
Copy link
Contributor

Choose a reason for hiding this comment

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

We usually uppercase controller names. 'TypeaheadController'

@ajoslin
Copy link
Contributor

ajoslin commented Jan 20, 2014

Absolutely love the changes, this is definitely the way to move forward. Controller exposes things, and options are now isolated and more maintainable in their own directive.

I just tested the demo though, and it doesn't select the item in the dropdown when I click it.

Eg I type 'Al', and then click Alabama, and it closes the dropdown menu but doesn't change the ngModel.

@chrisirhc
Copy link
Contributor Author

@ajoslin interesting, I must've broken something recently. I'll check back again tonight. Thanks for the review!

@bekos
Copy link
Contributor

bekos commented Jan 21, 2014

@ajoslin This one has landed?

@chrisirhc
Copy link
Contributor Author

Updated this PR. It's working now and I've tweaked it a bit more to use isolate scopes. Thinking of adding #1624 into this as well but it's a little tricky. Shall do another pass later this week.

@ajoslin
Copy link
Contributor

ajoslin commented Jan 28, 2014

Merge error again. When you update it to fix this error, I'll try to give it a good review quickly before it goes out of date again.

@quicksnap
Copy link

I'm testing out this fork currently after resolving the merge conflict. One test fails, but proceeding with build, seems to be working nicely.

Quick note, the example extension directive works as expected, but will throw an error if you select a typeahead via mouse click: https://gist.github.com/quicksnap/8740640

It seems to be due to scope.$apply() being called in our typeaheadOpenOnFocus, and then the click focus triggering here: https://github.com/chrisirhc/angular-ui-bootstrap/blob/feature/add-typeahead-controller/src/typeahead/typeahead.js#L150

I removed the scope.$apply() in the example extension directive, and it works well. Thanks!

Unfortunately, this depends on typeaheadEditable running after the post
linking function of the typeahead directive.

Note this and solve this later.
Maintain feature parity. Some duplicate code.
An attempt to make things clearer.
On blur event check if mouse is over the drop down before resetting
matches.

Introduce mouseIsOver in the controller but this will be removed
shortly.
Private variables that only deal with the pop-up go into the isolate
scope.
@chrisirhc
Copy link
Contributor Author

@ajoslin , updated the PR to fix the conflicts. Spoke to @pkozlowski-opensource and he feels that it'll be easier to review if it's broken up to multiple PRs, if you think the same as well, I'll do that.

@swayf
Copy link

swayf commented Mar 9, 2014

any dates, any plans to merge this awesome branch? :)

@miller-time
Copy link

@cindoum:
Is this directive working with angular-ui 0.6 + angular 1.2 ? I get
[$compile:ctreq] Controller 'typeahead', required by directive 'typeaheadOpenOnFocus', can't be found!

I'm also getting this error when I try the example using Angular 1.2 and ui.bootstrap 0.10. I'd really like to use this extension! Any news?

EDIT: Aha, I get it now. Your branch adds the controller necessary to do the require of 'typeahead'. Thanks for the awesome example any way, I look forward to seeing your code get merged.

@bdecarne
Copy link

+1

4 similar comments
@koen-serry
Copy link

+1

@mmadrigalac
Copy link

+1

@ValeryIvanov
Copy link

+1

@jalaniz1
Copy link

+1

@ValeryIvanov
Copy link

@chrisirhc do you think you could finish your work any time soon, or do you need help?

@thynctank
Copy link

Would love to see this brought into AUI bootstrap proper!

@herpoelaertw
Copy link

+1

1 similar comment
@uriklar
Copy link

uriklar commented Mar 5, 2015

+1

@wesleycho
Copy link
Contributor

There is a lot of good work here, but this is likely very outdated - @chrisirhc any plans on finishing this up? If not, we can open a separate issue with breaking up typeahead as a task.

@chrisirhc
Copy link
Contributor Author

@wesleycho , let's leave this out since we need some work for Angular 1.3 anyway.
This didn't get in because I couldn't get to writing tests.

@chrisirhc chrisirhc closed this Mar 16, 2015
@chrisirhc
Copy link
Contributor Author

I'm also hoping to move typeahead out of this repo since it's no longer core to bootstrap.

@Kikketer
Copy link

I really need this feature... are we simply delayed because we didn't have time to write a test?

@chrisirhc
Copy link
Contributor Author

Yes. Feel free to open a pull request with tests to speed things along.

@Kikketer
Copy link

Thank you, but I found it was much quicker to find an alternative (AngularStrap in this case).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.