-
Notifications
You must be signed in to change notification settings - Fork 19
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
Html render function #25
Html render function #25
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet! Just left some initial comments.
src/suggestions.js
Outdated
@@ -22,76 +22,74 @@ var Suggestions = function(el, data, options) { | |||
|
|||
this.list.draw(); | |||
|
|||
this.el.addEventListener('keyup', function(e) { | |||
this.el.addEventListener('keyup', function (e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove the paren formatting throughout this pr?
var indexOfQuery = indexString.lastIndexOf(this.query); | ||
while (indexOfQuery > -1) { | ||
var endIndexOfQuery = indexOfQuery + this.query.length; | ||
boldString = boldString.slice(0, indexOfQuery) + '<strong>' + boldString.slice(indexOfQuery, endIndexOfQuery) + '</strong>' + boldString.slice(endIndexOfQuery); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I'm following this. if this render
method allows a user to template a list item, would wrapping <strong>
be desired or expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the user provides a custom render
method, then that will be used instead. If the user does not provide a custom render function, then this default function will be used. The default function follows the existing functionality of wrapping the matching text in <strong>
tags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh dear me 🙈 I see now the existing functionality in the diff. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/suggestions.js
Outdated
@@ -66,8 +69,8 @@ Suggestions.prototype.handleKeyDown = function(e) { | |||
case 13: // ENTER | |||
case 9: // TAB | |||
if (!this.list.isEmpty()) { | |||
if (this.list.isVisible()) { | |||
e.preventDefault(); | |||
if (this.list.isVisible()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: seeing some extra whitespace here and the line below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💘
Exposes a render method that allows custom html to be displayed in the dropdown menu.