Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: highlight search term in labels in dropdown list #233

Merged
merged 3 commits into from
Feb 6, 2018

Conversation

benjaminlefevre
Copy link
Contributor

@benjaminlefevre benjaminlefevre commented Feb 3, 2018

#152

highlight search term in the items with a default style 'hightlight' (bold and underline text)

@coveralls
Copy link

coveralls commented Feb 3, 2018

Coverage Status

Coverage increased (+0.02%) to 90.958% when pulling fd19964 on BenjaminLefevreGithub:highlight_search_term into 7daf964 on ng-select:master.

Copy link
Member

@anjmao anjmao left a comment

Choose a reason for hiding this comment

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

@BenjaminLefevreGithub Thanks for contributing. I tested it locally. I like how it looks, but there are some issues. It would be great if you create custom directive which would be responsible for highlighting.

@@ -53,7 +53,7 @@
[class.marked]="item === itemsList.markedItem">

<ng-template #defaultOptionTemplate>
<span class="ng-option-label" [innerHTML]="item.label"></span>
<span class="ng-option-label" [innerHTML]="item.highlightedLabel"></span>
Copy link
Member

Choose a reason for hiding this comment

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

This approach is limited to default filter only and will not work if using custom [typeahead]. It would be better to create directive [ngOptionHighlight]="filterValue" which would be responsible for highlighting label.

<span [ngOptionHighlight]="filterValue" class="ng-option-label" [innerHTML]="item.label"></span>

Also users will be able to apply this directive even if they are using custom option templates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep good idea, done.

};
}

private _highlightLabelWithSearchTerm(option: NgOption, term: string, indexOfTerm: number) {
if (indexOfTerm > -1 && !option.selected && !option.disabled) {
Copy link
Member

Choose a reason for hiding this comment

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

It seems to add unneeded characters in some cases.
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed the end index was wrong on the third substring

@benjaminlefevre
Copy link
Contributor Author

@anjmao thanks for the review.

I fixed the bug (mistake on substring ;) the end index is not included :p)

And tried to make what you said: make a directive.
I updated also the template demos to take into account the directive.

I fixed also a bug discovered during my tests (https://ng-select.github.io/ng-select#/data-sources : console error log when trying to filter on the object array (true, 'Two', 3).

@Directive({
selector: '[ngOptionHighlight]'
})
export class NgOptionHighlightDirective implements OnChanges {
Copy link
Member

Choose a reason for hiding this comment

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

Looks nice, only add some unit tests for this directive and I can merge it. You can create new test file called ng-option-highlight.directive.spec.ts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem I will add some unit tests tonight

@benjaminlefevre
Copy link
Contributor Author

Just add unit test for the directive

@anjmao anjmao merged commit 493c349 into ng-select:master Feb 6, 2018
jakemdunn pushed a commit to jakemdunn/ng-select that referenced this pull request Oct 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants