-
Notifications
You must be signed in to change notification settings - Fork 22
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
[APP-4532] - Replace <SearchableSelect>
with svelte-select
#611
base: main
Are you sure you want to change the base?
Conversation
svelte-select
svelte-select
<SearchableSelect>
with svelte-select
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.
Quick initial thoughts. Very excited for this
$: normalizedOptions = options.map((option) => { | ||
if (typeof option === 'string') { | ||
return { value: option, label: option }; | ||
} | ||
|
||
return option; | ||
}); |
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.
thought (performance): This might be less of a big deal once Svelte 5 is in play, but this list of objects situation has given us a lot of grief in terms of unnecessary DOM operations from this component. I wonder if there's a path here - maybe in a future PR - where we lean more heavily into slots / snippets for stuff like option metadata rather than try to jam it all into props. Seems a little closer to how svelte-select
itself works
Overview
This PR replaces our
<SearchableSelect>
component with thesvelte-select
library. I did plenty of restructuring here to try to ameliorate the UX confusion and architectural problems that have arisen out of this component.Changelog
<SearchableSelect>
withsvelte-select
<SearchableSelect>
as<AutocompleteInput>
. The original name is confusing and led to the problematicexclusive
prop.exclusive
prop. This added unnecessary complexity - areas of app that use this should just use a<Select>
component.multiple
prop into<Multiselect>
component. This prop was overloading the behavior of this component.replace theproposing this later to move this through and avoid delays based on discussioncx
prop in favor ofclass
. I would like us to move toward this pattern for all prime components. Adding acx
prop introduces an unnecessary dependency considering any part of app can importcx
and pass the resulting string to a Prime component. This also follows the general pattern of "spread whatever you can into the underlying element whenever possible".Next steps
I'll immediately bump app and fix all breaking changes. I've scanned through what's needed - it won't be too bad.
svelte-select
supports therequired
prop.