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

docs: change search function from list.js to fuse.js #1653

Merged
merged 2 commits into from Mar 26, 2023

Conversation

korki43
Copy link
Contributor

@korki43 korki43 commented Mar 26, 2023

As discussed in #1432 (comment). Already includes the changes from #731.

Does not affect search performance (maybe improves it by a bit, but no significant changes [~100ms -> ~50ms]).

There is no difference to the original search results, except for search terms which use the 'extended search' features of fuse.js.

Preview:

@korki43 korki43 changed the title Change search from list.js to fuse.js docs: change search function from list.js to fuse.js Mar 26, 2023
@XhmikosR XhmikosR added docs Improvements or additions to documentation enhancement New feature or request labels Mar 26, 2023
package.json Outdated Show resolved Hide resolved
@XhmikosR
Copy link
Member

XhmikosR commented Mar 26, 2023

A couple of questions:

  1. do we lose the ability to implement docs: filter icons by URL search query #731? My guess is we are not but better ask beforehand NVM you included this here too :)
  2. is there an option to delay search and history change for a few milliseconds? There's no point to make instant updates since it wakes a while to type.
  3. any other options we should enable? Maybe we should keep sorting by name?
  4. do we really need fuzzy search? Doesn't the basic build suffice?

@XhmikosR
Copy link
Member

XhmikosR commented Mar 26, 2023

I rebased your branch, so only 2-3 things are left above :)

EDIT: Oh, actually I can't push to your branch...

@korki43
Copy link
Contributor Author

korki43 commented Mar 26, 2023

Oh, actually I can't push to your branch...

I changed it temporarily to avoid a conflict during the rebase. I just re-enabled it, so you can just pull my branch and push changes again.

@XhmikosR
Copy link
Member

Thank you for your contributions as usual! I updated my questions in my previous post.

@korki43
Copy link
Contributor Author

korki43 commented Mar 26, 2023

  1. Should be possible, I'll have a go at it
  2. It's still sorted by name. You can see the options here: https://fusejs.io/api/options.html
  3. The problem I had with the basic build was the missing AND operator for whitespaces.

@XhmikosR
Copy link
Member

Sounds good thanks!

So, I think it looks really good, love it ❤️

If we could go with the basic fuse.js build, that would be the best but if it's causing us issues, no big deal.

Debouncing maybe can be implemented later if you don't have time; search is fast it's just it's good practice to debounce things when we know there will be some delay.

As for sorting, I'm seeing weird sorting. I'm not saying it's wrong but it wort of feels weird. Must be fuse's filter score thing or something. Maybe since it's the default we should leave it for now, though.

image

@korki43
Copy link
Contributor Author

korki43 commented Mar 26, 2023

Just added debouncing.
You're right about the sorting. I'll look into it.

@@ -45,7 +45,11 @@
window.history.replaceState(null, null, newUrl)
}

searchInput.addEventListener('input', () => search(searchInput.value))
Copy link
Member

Choose a reason for hiding this comment

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

please use let. At some point I need to backport the eslint config from the main repo...

Now, regarding your solution, maybe https://levelup.gitconnected.com/debounce-in-javascript-improve-your-applications-performance-5b01855e086 ?

@XhmikosR
Copy link
Member

Maybe it's fuzzy matching in effect https://fusejs.io/api/options.html#threshold or ignoreLocation?

We could leave it as is, it's not bad per se, it's just different from what we had.

@XhmikosR
Copy link
Member

Oh, and it's seems scrollIntoView() is missing I think.

@korki43
Copy link
Contributor Author

korki43 commented Mar 26, 2023

Yes, the sorting was using the filter scoring. I think it's better to see similar icons grouped together than to see which icon matches your search the most so I changed it.

The sorting is now just like with the old search.

@korki43
Copy link
Contributor Author

korki43 commented Mar 26, 2023

Fuzzy search is currently disabled (treshold: 0) so it'll return the same results as before. Theoretically we could enable it to a certain degree to create a more error-resistant search.

korki43 and others added 2 commits March 26, 2023 19:25
use debounced fuse.js extended search in search function
@XhmikosR
Copy link
Member

Cool, let's land this and we can improve it later. Do not hesitate to submit a PR and CC me later :)

@XhmikosR XhmikosR merged commit 0b52cc6 into twbs:main Mar 26, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to documentation enhancement New feature or request
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants