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

Fixed filtering of suggestions with ranges #762

Merged
merged 1 commit into from
Oct 15, 2023

Conversation

mauricioszabo
Copy link
Contributor

Previously, autocomplete suggestions with .ranges property was being filtered by the same rules as previous autocomplete - basically, by matching the first character.

Unfortunately, that might not be the case anymore - with this API, we are choosing which range to replace, so the first char is not relevant anymore (and considering we're matching up multiple ranges, it may be difficult to define which range, or ranges, we want to match).

This PR fixes that by not filtering anything that have ranges only with the first char - the other filter (the "fuzzy" match) will still be aplied.

@confused-Techie
Copy link
Member

While these changes look awesome, it seems that the fallback behavior, for when suggestion.ranges isn't defined, is not identical to the previous code. (Which I'm assuming was the original intention).

Would we want to change line 384 to include the !prefixIsEmpty check that accompanied checking if the suggestions prefix first char and text first char are identical. such as:

const keepMatching = suggestion.ranges || (!prefixIsEmpty && suggestionPrefix[0].toLowerCase() === text[0].toLowerCase());

But otherwise this looks great! Love to see that you've added a test for it as well

@mauricioszabo
Copy link
Contributor Author

Actually, @confused-Techie, they are identical. The old code was just two if, one checking if prefixIsEmpty and the other negating the check.

What I did was to move the second check into an else branch, so that !prefixIsEmpty is already checked

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

Ahh thanks for clarifying @mauricioszabo I somehow missed that.

Otherwise this looks good, and since we can see tests are happy here, I say lets get this one in!

@confused-Techie confused-Techie merged commit 34a5c5b into master Oct 15, 2023
@mauricioszabo mauricioszabo deleted the fix-autocomplete-v5 branch October 16, 2023 14:30
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