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

Moving fuzzy-native to core #774

Merged
merged 23 commits into from
Dec 13, 2023
Merged

Moving fuzzy-native to core #774

merged 23 commits into from
Dec 13, 2023

Conversation

mauricioszabo
Copy link
Contributor

Creating a atom.fuzzyMatcher API containing setCandidates(arrayOfCandidates), score(candidate, stringToMatch) and match(candidate, stringToMatch).

Also fixed fuzzy-finder, command-palette, and autocomplete-plus to use this API instead of adding hundreds of different fuzzy-finder dependencies, each on its own version

@asiloisad
Copy link
Contributor

asiloisad commented Oct 18, 2023

I'm super glad this PR. It has to be done long time ago as many packages use it 👍

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.

Seriously this looks fantastic!

I love the way you've exported the fuzzyMatcher object with all of it's functions as a part of that one object. Really helps to compartmentalize the whole thing, and I'll absolutely be taking the same queue for the markdown portion.

Otherwise, this is amazing to see such a simple drop in replacement for the functionality in so many packages, (plus glad you updated the relevant settings on each one) so it's awesome to see.

I'd be happy getting this one merged, although of course I still have some work to do over on my portion of the branch, maybe for ease of reviewing my markdown changes we wait until I'm done over there prior to merging?

@mauricioszabo
Copy link
Contributor Author

Sure, @confused-Techie, let's wait for your portion and then we can merge it :)

Copy link
Member

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

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

I'm "lite-approve"-ing this, since it has popular demand behind it.

Once again, it seems a bit above my ability to review every bit of the diff, but I offer to help with troubleshooting if there is any issue with it post-merge.

Base automatically changed from ui-api to master November 9, 2023 04:48
@confused-Techie
Copy link
Member

@mauricioszabo If you want to go ahead and take this one out of draft status, feel free my other PR has been approved.

Also the failing CI here seems to be from a mistake on my end, that's now been resolved.

Also just now realized with this PR targeting my branch specifically may be problematic, sorry about that. Hopefully you can just target the main branch now, but not sure what's easiest on your end

@mauricioszabo
Copy link
Contributor Author

@confused-Techie great, it already updated to master so it's all good :)

I'll just add some JSDocs too that I forgot

src/ui.js Outdated Show resolved Hide resolved
@mauricioszabo mauricioszabo marked this pull request as ready for review November 24, 2023 01:06
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.

This looks good to me, approval part 2

@mauricioszabo mauricioszabo merged commit f3b9bd8 into master Dec 13, 2023
99 checks passed
@mauricioszabo mauricioszabo deleted the filtering-api branch December 13, 2023 03:01
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.

None yet

4 participants