Add search functionality

pamelafox / font-previewer-extension

Google Font Previewer Chrome Extension

https://chrome.google.com/webstore/detail/engndlnldodigdjamndkplafgmkkencc

69 stars 14 forks source link

Add search functionality #14

Closed jerboa88 closed 2 years ago

jerboa88 commented 2 years ago

Hey Pamela. I'm not sure if you're still maintaining this project but I've added search functionality for fonts (#9, #10), fixed the subset dropdown, and made some other small tweaks.

You can enter multiple font families separated by commas and only fonts with the selected subsets are shown. Searching takes under 10ms on my machine so performance isn't an issue. I have tested this and it doesn't appear to break any existing functionality.

Other things:

Subset dropdown
- Fixed the dropdown as it was calling the wrong event handler on change and doing nothing
- Set the local storage key to lsSubset as previously it was undefined, although I haven't really tested whether local storage is working properly as it is out of scope
- Added all entry to dropdown so that users can view all subsets
Added protocol to front of fontUrl

henningko commented 2 years ago

Awesome, just what I was looking for :-) I might need to look into changing the weights to numbers.

pamelafox commented 2 years ago

I can definitely see how search functionality would be helpful given how many fonts there are these days, thanks for tackling that! Is it possible to split the PR up into the various changes? It looks like there's also some JS modernization (var to const, for-i to for-of, etc), though that might have all been related to the search feature.

I'd rather merge each feature/fix separately so that it's easier to revert if needed. Smaller PRs are also easier to review.

jerboa88 commented 2 years ago

Sure, I can split it up. I got a bit carried away with the other changes haha. I'll leave the search implementation in this PR and make another for fixing the subset dropdown after the merge (those changes depend on what I wrote here).

jerboa88 commented 2 years ago

Okay, so I've stripped out everything that is not related to search here.

There are some changes to onFontsLoad() and loadVisibleFonts() methods because I've basically removed allFontNames in favor of the fonts object since this already contains all the info we need to do filtering, etc. The methods related to search should be pretty self-explanatory.

pamelafox commented 2 years ago

Generally looks good, just a few comments.

jerboa88 commented 2 years ago

Sorry, where are your comments?

pamelafox commented 2 years ago

@jerboa88 Dah, sorry, forgot to submit them.

pamelafox commented 2 years ago

Great, merging!