-
-
Notifications
You must be signed in to change notification settings - Fork 57
feat: Enable fuzzy search for collection auto-add #2871
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
Conversation
f6dfaa3
to
a6ed3e3
Compare
a6ed3e3
to
20d3c97
Compare
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.
Works brilliantly, great improvement!
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.
Overall this is an improvement for sure, but there's a few things I'm a little confused about and a couple of bugs/unexpected behaviours I was able to find.
private readonly searchTask = new Task(this, { | ||
task: async ([names], { signal }) => { | ||
if (!names || signal.aborted) { | ||
return; | ||
} | ||
|
||
return new Fuse(names, { threshold: 0.4, minMatchCharLength: 2 }); | ||
}, | ||
args: () => [this.searchValuesTask.value] as const, | ||
}); |
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.
I'm not sure I understand why this is a Task
— it seems to me that this could go in a willUpdate
, because all of the code here is synchronous. As such, the pending state on line 238 is never rendered, since the task always immediately successfully completes whenever it's run.
return; | ||
} | ||
|
||
return new Fuse(names, { threshold: 0.4, minMatchCharLength: 2 }); |
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.
Should minMatchCharLength
be MIN_SEARCH_LENGTH
? As is, when starting to search for something, you don't get any results until you've typed more than two characters, even though when you've only typed one character you get both a) results back from the search prefix endpoint, if there's any collections starting with the character you've entered, and b) a message that says "no matching collections found", which isn't true.
private readonly searchResultsTask = new Task(this, { | ||
task: async ([searchByValue, hasSearchStr], { signal }) => { | ||
if (!hasSearchStr) return []; | ||
const data = await this.fetchCollectionsByPrefix(searchByValue, signal); | ||
let searchResults: Collection[] = []; | ||
if (data?.items.length) { | ||
searchResults = this.filterOutSelectedCollections(data.items); | ||
} | ||
return searchResults; | ||
}, | ||
args: () => [this.searchByValue, this.hasSearchStr] as const, | ||
}); |
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.
Correct me if I'm wrong, but I don't think the results of this are used anywhere, are they?
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.
Yeah, it looks like currently onSearchInput
serves to re-render the component, and nothing else, so it could functionally be replaced with this.requestUpdate()
Resolves #2846
Changes
Manual testing
Screenshots