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

Implemented autocomplete functionality in open bot dialog #1510

Merged
merged 1 commit into from
May 6, 2019

Conversation

tonyanziano
Copy link
Contributor

@tonyanziano tonyanziano commented May 2, 2019

One of #1443

===

Don't worry, I'll squash all of the commits.

image

image

image

@mewa1024 I just used the styling from the Split Button drop down specs

// CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.

// from https://github.com/bevacqua/fuzzysearch
private fuzzysearch(needle: string, haystack: string) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't know if the above copyright and link to source will be compliant, but we can discuss it.

@tonyanziano tonyanziano force-pushed the toanzian/autocomplete branch 2 times, most recently from 68a855a to de17c0f Compare May 3, 2019 19:51
@tonyanziano tonyanziano marked this pull request as ready for review May 3, 2019 19:51
@coveralls
Copy link

coveralls commented May 3, 2019

Coverage Status

Coverage increased (+0.4%) to 57.642% when pulling 01be45f on toanzian/autocomplete into d5f6766 on master.

justinwilaby
justinwilaby previously approved these changes May 3, 2019
Copy link
Contributor

@justinwilaby justinwilaby left a comment

Choose a reason for hiding this comment

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

Some recommendations but looks good overall

return this.props.label ? `auto-complete-textbox-${this.state.id}` : undefined;
}

private get filteredItems(): string[] {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would expect the matches to have some sort of emphasis on the searched value. This can be achieved through using a <strong> tag for the matched string:

...
return filteredItems.map(item => item.replace(new RegExp(this.value, 'gi'), `<strong>${this.value}</strong>`);

Then use dangerouslySetInnerHtml on the <li> tag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be a nice visual improvement but it might not work so well with this search algorithm because it just checks for the succession of characters in string and returns true if it exists.

For example:

ir would match on international because there is an i in the string, followed by an r. So the meaningful way to emphasize that would probably be to bold everything between the first "i" and the last "r" so you would be left with international.

The method you suggested would provide some strange results in some words:

er matches on Herbert so the end result would be Herbert

o matches on localhost:3948/api/orders so the end result would be localhost:3948/api/orders with 3 bolded o's

In my opinion, bolding every character between the first and last matched characters would be the way to go, but that seems like a QoL improvement that could be added in a later PR and not critical to this first implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened an issue to track this work: #1518

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. I did't look closely at the fuzzy search function to see this is what it's doing. Maybe an alternative would be to implement the emphasis for sub strings that are exact sequential matches and then sort them so they are first in the list.

Thoughts?

Also, although very long lists are unlikely, have we considered the performance implications of the loop nesting in the search algo for long lists? How would a user remove items that may have gotten in there from a typo or that are no longer needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That strategy for highlighting sounds good to me!

As for the performance with long lists, there are two trivial solutions I can think of:

  • put a limit on the number of saved items we feed into the component (easy to manage in the context of saved bot endpoints, but would have to be managed for each autocomplete placed throughout the app)
  • swap out the current search algorithm for a more performant one (very easy fix)

The list of searchable items is provided by the parent of the widget, so any item removal / clear functionality would have to be implemented by the parent. It would be very straight forward in this case and we would just need to add a new action handler to the savedBotUrls reducer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened an issue to track work for list item removal / clearing UI: #1525

@cwhitten cwhitten merged commit fad7c69 into master May 6, 2019
@cwhitten cwhitten deleted the toanzian/autocomplete branch May 6, 2019 19:03
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.

4 participants