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

feat: Issue #2369 Regarding Fuzzy Search #2437

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

wel013
Copy link

@wel013 wel013 commented Dec 5, 2024

The PR fulfills these requirements: (check all the apply)

  • It's submitted to the main branch.
  • When resolving a specific issue, it's referenced in the PR's title (e.g. feat: Add a button #xxx, where "xxx" is the issue number).
  • When resolving a specific issue, the PR description includes Closes #xxx, where "xxx" is the issue number.
  • If changes were made to ui folder, unit tests (make test) still pass.
  • New/updated tests are included
    This is regarding the issue of someone suggested to be able to "toggle" the fuzzy search for dropdowns and allow for exact search. The main change was one more boolean attribute for the interface in dropdown.

Added test cases for using exact (instead of fuzzy search)
Edited the interface to allow exactsearch
Added a function for searching with exact terms of top of the fuzzysearch
@wel013 wel013 requested review from lo5 and mturoci as code owners December 5, 2024 16:24
@wel013 wel013 closed this Dec 7, 2024
@mturoci
Copy link
Collaborator

mturoci commented Dec 8, 2024

Why did you close the PR @wel013? It looks good at the first sight, just need to get some time to go through it thorougly.

@wel013
Copy link
Author

wel013 commented Dec 8, 2024

Hi, I believe I closed this possibly due to an accidental mistake on my end, sorry about it!

@wel013 wel013 reopened this Dec 8, 2024
Copy link
Collaborator

@mturoci mturoci left a comment

Choose a reason for hiding this comment

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

Thanks @wel013!

Looks like you didn't generate types via make generate. Also please add a demo video of the new feature in action.

Comment on lines +64 to +65
/**Whether the search will be exact or fuzzy */
exactSearch?: B
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/**Whether the search will be exact or fuzzy */
exactSearch?: B
/** Whether the dropdown search will be exact or fuzzy. Defaults to True. */
fuzzy_search?: B

Comment on lines +27 to +32
// parts / utils.ts
export const exactsearch = (searchTerm: string, itemText: string): boolean => {

// Convert both strings to lowercase for case-insensitive comparison
return itemText.toLowerCase() === searchTerm.toLowerCase(); // Exact match
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for a separate function since it's so simple, can be inlined.

Comment on lines +183 to +184
? exactsearch(i.text, newVal) // Assuming exactsearch is a function for exact matching
: fuzzysearch(i.text, newVal), // Assuming fuzzysearch is a function for fuzzy matching
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for extra comments.

Suggested change
? exactsearch(i.text, newVal) // Assuming exactsearch is a function for exact matching
: fuzzysearch(i.text, newVal), // Assuming fuzzysearch is a function for fuzzy matching
? exactsearch(i.text, newVal)
: fuzzysearch(i.text, newVal),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants