-
-
Notifications
You must be signed in to change notification settings - Fork 331
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/Autocomplete Generics #2021
feat/Autocomplete Generics #2021
Conversation
|
@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 ↗︎
|
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. |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love the proposed changes, great job! I pushed a couple of changes when it came to annotating types and some variable names.
One thing I wanted to mention is that since we're already modifying the type of value
into a generic, we may as well do the same for meta
.
Additionally, keywords
looks to accept a string
type, despite being labeled as any
. Note the comment for it: /** Provide a comma separated list of keywords. */
. What are your thoughts on this @endigo9740? I looked back in the original PR that introduced the component and didn't see any conversations about keeping keywords
as any
, so I'm assuming this is an any
type that slipped between the cracks.
@AdrianGonz97 Per |
@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 |
Co-authored-by: CokaKoala <[email protected]>
sites/skeleton.dev/src/routes/(inner)/components/autocomplete/+page.svelte
Outdated
Show resolved
Hide resolved
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
Linked Issue
Closes #1188
Description
Fixes generics on values and other props in Autocomplete component
Changsets
feat: autocomplete generics
Instructions: Changesets automate our changelog. If you modify files in
/packages/skeleton
, runpnpm changeset
in the root of the monorepo, follow the prompts, then commit the markdown file. Changes that add features should beminor
while chores and bugfixes should bepatch
. Please prefix the changeset message withfeat:
,bugfix:
orchore:
.Checklist
Please read and apply all contribution requirements.
dev
branch (NEVERmaster
)docs/
,feat/
,chore/
,bugfix/
pnpm check
pnpm format
pnpm test