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

typeahead expects li in template #5848

Closed
frfancha opened this issue Apr 26, 2016 · 5 comments
Closed

typeahead expects li in template #5848

frfancha opened this issue Apr 26, 2016 · 5 comments

Comments

@frfancha
Copy link

The code:
target = popUpEl.find('li')[scope.activeIdx];
target.parentNode.scrollTop = target.offsetTop;
in typeahead.js in master fails (target is undefined) if the template doesn't contains -li- (we use -tr- instead of -li- and -table- instead of -ul- by typeahead-popup-template-url).

When the developper tool is not opened in the browser everything runs fine despite the error.

If using -li- is officialy required in custom templates, then this is a feature request to allow other html elements, and if -li- is already allowed today then it is to report the bug.

@frfancha frfancha changed the title typeahead expects li n template typeahead expects li in template Apr 26, 2016
@icfantv
Copy link
Contributor

icfantv commented Apr 26, 2016

@frfancha, please be respectful of our time by adhering to our issue template.

additionally, please use MD formatting to make your issue text more readable.

@pkozlowski-opensource
Copy link
Member

This is a valid issue. We shouldn't be doing DOM querying like this as it limits possibilities for template swapping.

@pkozlowski-opensource
Copy link
Member

Offensive code is here:

target = popUpEl.find('li')[scope.activeIdx];
target.parentNode.scrollTop = target.offsetTop;
break;
case 40:
scope.activeIdx = (scope.activeIdx + 1) % scope.matches.length;
scope.$digest();
target = popUpEl.find('li')[scope.activeIdx];

@wesleycho
Copy link
Contributor

We probably should change it to query for a class instead of a tag.

@RobJacobs
Copy link
Contributor

Was looking at creating a PR for this, was wondering if using:

[role="option"]

for the query selector would work.

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

Successfully merging a pull request may close this issue.

6 participants