-
Notifications
You must be signed in to change notification settings - Fork 4
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
adjust fuse threshold in offline search to better filter out irrelevant results #1183
Conversation
Netlify Deployments: |
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.
Device | URL |
---|---|
mobile | https://ocw-hugo-themes-www-pr-1183--ocw-next.netlify.app/ |
Device | URL |
---|---|
mobile | https://ocw-hugo-themes-www-pr-1183--ocw-next.netlify.app/search/ |
Device | URL |
---|---|
mobile | https://ocw-hugo-themes-course-v2-pr-1183--ocw-next.netlify.app/ |
Not what you expected? Are your scores flaky? GitHub runners could be the cause.
Try running on Foo instead
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.
In addition to the comment I left, one other requested change: Move const searchResultsSection = document.getElementById("search-results")
above performSearch
.
Currently, the else
block throws an error since searchResultsSection
is not defined. Gosh, I forgot how easy these errors are in un-linted files.
@@ -73,7 +73,7 @@ <h3 class="col-3 text-center m-auto">Start typing to search</h3> | |||
const Fuse = window.Fuse | |||
const searchOptions = { | |||
shouldSort: true, | |||
threshold: 0.4, | |||
threshold: 0.2, | |||
location: 0, | |||
distance: 100, | |||
matchAllTokens: true, |
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.
Requested Change: matchAllTokens
: I don't think exists anymore. It's not in the docs and changelog says it was removed
Suggestion
I think we should ditch location
and distance
. Remove those options, and add ignoreLocation
.
With { threshold: 0.2, distance: 100, location: 0 }
a match only counts if it occurs (starts??) within the first 20 characters, which really isn't that many. For example:
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.
@gumaerc Did you want to add ignoreLocation: true
? Currently, fuse is using the default values for distance/loc, so "Geographic Information Systems" still shows limited results.
Requested change: As mentioned: Move const searchResultsSection = document.getElementById("search-results") above performSearch.
Currently, the else
block is throwing when there are no results.
176cb8a
to
2621e01
Compare
@ChristopherChudzicki Whoops, I missed that part but that makes sense. It should be all set now. |
What are the relevant tickets?
Closes #1182
Description (What does it do?)
This PR simply adjusts the
threshold
value in the FuseJS configuration for offline search inwww-offline
to 0.2 from 0.4. After some testing, this seems to be a good place to set it in order to filter out irrelevant results.Screenshots (if appropriate):
How can this be tested?
Follow the same instructions as #1175, but pay extra attention to the results and verify that irrelevant results have been minimized