feat/Autocomplete Generics - #2021

⚠️ No Changeset found

Latest commit: bbaa2bf

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@HugeLetters is attempting to deploy a commit to the Skeleton Labs Team on Vercel.

A member of the Team first needs to authorize it.

Thanks for submitting the draft @HugeLetters.

@AdrianGonz97 and @ryceg are our resident Typescript experts, so I'm going to ping them here to provide feedback asap.

To make sure we're on the same page, I mentioned in the main thread this approach might be something we want to extend to Modals and other Skeleton features too. I'm fine if we want to take a piecemeal approach to that, as long as these aren't lost in the shuffle.

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Thanks for submitting the draft @HugeLetters.

@AdrianGonz97 and @ryceg are our resident Typescript experts, so I'm going to ping them here to provide feedback asap.

To make sure we're on the same page, I mentioned in the main thread this approach might be something we want to extend to Modals and other Skeleton features too. I'm fine if we want to take a piecemeal approach to that, as long as these aren't lost in the shuffle.

Sure, once this gets approved and I get a better understanding of kind of code style is appreciated here I'll try to work on other components too.
Btw, what should the procedure be on minor improvements? Like here there's an unnecessary array copying or here when there's no limit set.

Btw, what should the procedure be on minor improvements? ...

We keep scope small and limited in PRs. This helps keep changes localized and not grow too large. For additional issues it's worth creating new issues and dedicated PRs for each change.

I will also advise you to browse our issue backlog. There's a number of issues folks have with the autocomplete component specifically. It's a feature implemented by me, but not one I've had a lot of prior experience with, so I think there's definitely some room for improvement. At this point we're reaching a point a more consorted effort to improve the feature top to bottom might be in order.

If you're not already on Discord, I might recommend joining and jumping into the #contributor channel. This is a great place to discuss and plan bigger ideas. That or creating a Discussions section here on GitHub. Once a set plan is established we can generate one or more issues against that.

@AdrianGonz97 Per keywords, yes those should be a string ideally. Offhand I can't recall if we're parsing those, but they will be read as a string in the end.

@endigo9740 Thanks! I thought as much. Looking at the code, we're not parsing it at all and the only time it's used is when everything is being strinigified for search - see here.

In that case, I think it's safe to change that any to a string then.

Hey @AdrianGonz97 not sure if you saw but @HugeLetters had a few follow up questions above. If we can get these addressed and give me a thumbs up when this is approved and ready to merge.

Excellent changes and thanks for the update!

I've pushed a small change that adjusts the type for options so that it's appearance in the docs site makes a little more sense. And with that, everything else looks great and I think this PR is ready to be merged.

Great! Somebody needs to authorize Vercel's deployment to follow through.