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

Update Fuse.js configuration to adjust search threshold #3288

Merged

Conversation

precious-onyenaucheya-ons
Copy link
Contributor

@precious-onyenaucheya-ons precious-onyenaucheya-ons commented Aug 8, 2024

What is the context of this PR?

This PR fixes #3347

This PR updates the Fuse.js configuration to address an issue where only the first 30 characters in the list seem to be searched.

The previous threshold setting of 0.2 was too restrictive, causing the search to miss relevant entries when queries became longer. With this update, users can now set the search threshold between 0 and 1, allowing for greater flexibility and improving search accuracy across different query lengths.

How to review this PR

Describe the steps required to test the changes (include screenshots if appropriate).

  1. Run the search functionality with the updated configuration.
  2. Perform a search for "South Georgia and the South Sandwich Islands" with the updated threshold.
  3. Verify that the search result appears when searching for "South Sa" and similar queries.

Checklist

This needs to be completed by the person raising the PR.

  • I have selected the correct Assignee
  • I have linked the correct Issue

@precious-onyenaucheya-ons precious-onyenaucheya-ons added Enhancement Change of existing feature Spike Requires investigation outside BAU work labels Aug 8, 2024
@precious-onyenaucheya-ons precious-onyenaucheya-ons requested a review from a team August 8, 2024 12:58
Copy link

netlify bot commented Aug 8, 2024

Deploy Preview for ons-design-system-preview ready!

Name Link
🔨 Latest commit c29b7c2
🔍 Latest deploy log https://app.netlify.com/sites/ons-design-system-preview/deploys/672b467bca8de300081e8152
😎 Deploy Preview https://deploy-preview-3288--ons-design-system-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@precious-onyenaucheya-ons precious-onyenaucheya-ons changed the title Update Fuse config to increase search threshold Update Fuse.js Configuration to Adjust Search Threshold Aug 8, 2024
@precious-onyenaucheya-ons precious-onyenaucheya-ons marked this pull request as draft August 9, 2024 09:18
@rmccar rmccar changed the title Update Fuse.js Configuration to Adjust Search Threshold Update Fuse.js configuration to adjust search threshold Sep 19, 2024
@rmccar rmccar added the Do not merge Don't merge this PR until this label is removed label Oct 2, 2024
@rmccar rmccar removed the Do not merge Don't merge this PR until this label is removed label Oct 9, 2024
@precious-onyenaucheya-ons precious-onyenaucheya-ons marked this pull request as ready for review October 14, 2024 10:58
@rmccar
Copy link
Contributor

rmccar commented Oct 17, 2024

The description now needs updating with the latest changes

src/components/autosuggest/_macro-options.md Outdated Show resolved Hide resolved
src/components/autosuggest/_macro.njk Outdated Show resolved Hide resolved
src/components/autosuggest/example-autosuggest-country.njk Outdated Show resolved Hide resolved
src/components/autosuggest/autosuggest.ui.js Outdated Show resolved Hide resolved
src/components/autosuggest/autosuggest.ui.js Outdated Show resolved Hide resolved
@rmccar
Copy link
Contributor

rmccar commented Oct 17, 2024

I think there may be an issue with this not working as expected, I upped the threshold to 0.8 in the example and was still getting the same results. So you may need to make sure that is being passed into the config correctly

rmccar
rmccar previously approved these changes Oct 18, 2024
@rmccar rmccar dismissed their stale review October 18, 2024 13:00

Noticed something that needs to be fixed

@precious-onyenaucheya-ons precious-onyenaucheya-ons removed the Spike Requires investigation outside BAU work label Oct 22, 2024
@admilne
Copy link
Contributor

admilne commented Oct 27, 2024

@precious-onyenaucheya-ons @rmccar

Sorry, I ran out of time to look at this on Friday after running into some git permission issues Precious is aware of but I wanted to take a look before you have stand up in the morning. I'm not sure if I've misunderstood the ticket and the requirements - it wasn't entirely clear for me but this doesn't look like it's working to me. Could be I'm not looking at it correctly but once I hit around 32 characters in the search box it no longer returns the results I'd expect. I've taken some screen shots to illustrate what I experienced.

  • "South Georgia and the South San" - returns what i'd expect
  • "South Georgia and the South Sand" - doesn't seem to return anything
  • "South Georgia and the South Sandw" - gives me some results, but doesn't seem to be based on anything I've searched for
  • "South Georgia and the South Sandwich Island" - still appears to give me results not based on my search
  • "South Georgia and the South Sandwich Islands" - suddenly seems to kick back in to life and return an expected result but along with unexpected results too.

I tried playing with the thresholds but this didn't seem to make any difference to the issues I've described above.

Screenshot 2024-10-27 at 22 06 23 Screenshot 2024-10-27 at 22 06 34 Screenshot 2024-10-27 at 22 06 51 Screenshot 2024-10-27 at 22 07 09 Screenshot 2024-10-27 at 22 07 20

@adi-unni
Copy link
Contributor

adi-unni commented Oct 28, 2024

I'd also like to second @admilne's findings. I've also done some additional testing with some of my own examples.
I created a new JSON file with only examples of search terms which are longer than 32 characters. It seems like the search threshold doesn't change the results when typing. Whats more worrysome is the fact that words outside of the 32 character soft limit aren't picked up by fuse.

image

In this picture, you can see this is a search term. You can also see the word characters in the search terms which is the 54th character in the field

image

Characters does not return anything in this scenario

This might seem like an edge case but say there was a scenario where a user wanted to match this:

Ultra HD 4K Smart Television Model XYZ1234567890

based on the model number which is outside of 32 characters

You can test my changes in: feature/3282/increase-search-threshold-adi

Edit: Actually in even more testing, back to the original example (testing in countries-multiple), if you type "sandwich islands", you'd expect "South Georgia and the South Sandwich Islands" to appear but nothing appears

Edit 2: Even weirder, if you search for "thi" in the search box in my example, you get matches for "thi" beyond 32 characters? Not sure if this is because its previously matched "thi" at the beginning.

image

@rmccar rmccar dismissed their stale review October 28, 2024 13:29

Needs rereview

@rmccar
Copy link
Contributor

rmccar commented Oct 29, 2024

I'd also like to second @admilne's findings. I've also done some additional testing with some of my own examples. I created a new JSON file with only examples of search terms which are longer than 32 characters. It seems like the search threshold doesn't change the results when typing. Whats more worrysome is the fact that words outside of the 32 character soft limit aren't picked up by fuse.

image In this picture, you can see `this` is a search term. You can also see the word `characters` in the search terms which is the 54th character in the field image Characters does not return anything in this scenario

This might seem like an edge case but say there was a scenario where a user wanted to match this:

Ultra HD 4K Smart Television Model XYZ1234567890

based on the model number which is outside of 32 characters

You can test my changes in: feature/3282/increase-search-threshold-adi

Edit: Actually in even more testing, back to the original example (testing in countries-multiple), if you type "sandwich islands", you'd expect "South Georgia and the South Sandwich Islands" to appear but nothing appears

Edit 2: Even weirder, if you search for "thi" in the search box in my example, you get matches for "thi" beyond 32 characters? Not sure if this is because its previously matched "thi" at the beginning.

image

These issues seem to be fixed by changing the distance to 500 in my testing without changing the threshold at all. So maybe we need to parameterise the distance not the threshold. But the issues Andrew highlighted still seem to be an issue. There might be an issue with the length of the query string entered which would cause the issues Andrew has found. This all needs bit more testing to make sure we get a balance between the distance and threshold. Testing with ignoreLocation might also be something we want to look into.

@adi-unni adi-unni self-requested a review October 29, 2024 13:37
@rmccar
Copy link
Contributor

rmccar commented Oct 31, 2024

Ive found an issue with the component search on the DS. It finds the list of components when you start typing the name of a component but no longer goes to that page when you click the suggestion. There is this error in the console:
Screenshot 2024-10-31 at 09 48 00

But the issues Andrew found do seem to be fixed now

Copy link
Contributor

@admilne admilne left a comment

Choose a reason for hiding this comment

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

LGTM and looks like it's working as expected

@precious-onyenaucheya-ons precious-onyenaucheya-ons merged commit 7d9b0ac into main Nov 7, 2024
25 checks passed
@precious-onyenaucheya-ons precious-onyenaucheya-ons deleted the feature/3282/increase-search-threshold branch November 7, 2024 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Change of existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend search functionality for Autosuggest component
5 participants