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

[APP-4532] - Replace <SearchableSelect> with svelte-select #611

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

micheal-parks
Copy link
Member

@micheal-parks micheal-parks commented Dec 17, 2024

Overview

This PR replaces our <SearchableSelect> component with the svelte-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

  • Replace <SearchableSelect> with svelte-select
  • Rename <SearchableSelect> as <AutocompleteInput>. The original name is confusing and led to the problematic exclusive prop.
  • Remove exclusive prop. This added unnecessary complexity - areas of app that use this should just use a <Select> component.
  • Split out multiple prop into <Multiselect> component. This prop was overloading the behavior of this component.
  • replace the cx prop in favor of class. I would like us to move toward this pattern for all prime components. Adding a cx prop introduces an unnecessary dependency considering any part of app can import cx 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". proposing this later to move this through and avoid delays based on discussion

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.

  • Todos in app about adding a required field can be removed, svelte-select supports the required prop.

@micheal-parks micheal-parks changed the title [APP-4532] - Replace Searchable Select with library [APP-4532] - Replace Searchable Select with svelte-select Dec 18, 2024
@micheal-parks micheal-parks changed the title [APP-4532] - Replace Searchable Select with svelte-select [APP-4532] - Replace <SearchableSelect> with svelte-select Dec 18, 2024
Copy link
Member

@mcous mcous left a 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

packages/core/package.json Outdated Show resolved Hide resolved
packages/core/src/lib/modal.svelte Outdated Show resolved Hide resolved
packages/core/src/lib/select/index.ts Outdated Show resolved Hide resolved
packages/core/tailwind.config.ts Outdated Show resolved Hide resolved
Comment on lines 52 to 58
$: normalizedOptions = options.map((option) => {
if (typeof option === 'string') {
return { value: option, label: option };
}

return option;
});
Copy link
Member

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

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.

2 participants