Skip to content

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

Merged
XhmikosR merged 2 commits into
twbs:mainfrom
kiehnek:new-search
Mar 26, 2023
Merged

docs: change search function from list.js to fuse.js#1653
XhmikosR merged 2 commits into
twbs:mainfrom
kiehnek:new-search

Conversation

@kiehnek

@kiehnek kiehnek commented Mar 26, 2023

Copy link
Copy Markdown
Contributor

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:

@kiehnek kiehnek 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
Comment thread package.json Outdated
@XhmikosR

XhmikosR commented Mar 26, 2023

Copy link
Copy Markdown
Member

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

XhmikosR commented Mar 26, 2023

Copy link
Copy Markdown
Member

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

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

@kiehnek

kiehnek commented Mar 26, 2023

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Member

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

@kiehnek

kiehnek commented Mar 26, 2023

Copy link
Copy Markdown
Contributor Author
  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
Copy Markdown
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

@kiehnek

kiehnek commented Mar 26, 2023

Copy link
Copy Markdown
Contributor Author

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

Comment thread docs/assets/js/search.js Outdated
window.history.replaceState(null, null, newUrl)
}

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

Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread docs/assets/js/search.js
@XhmikosR

Copy link
Copy Markdown
Member

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

@kiehnek

kiehnek commented Mar 26, 2023

Copy link
Copy Markdown
Contributor Author

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.

@kiehnek

kiehnek commented Mar 26, 2023

Copy link
Copy Markdown
Contributor Author

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.

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

Copy link
Copy Markdown
Member

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

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.

2 participants