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

Add searchable select #339

Merged
merged 21 commits into from
Aug 29, 2023
Merged

Conversation

DTCurrie
Copy link
Member

@DTCurrie DTCurrie commented Aug 24, 2023

Adds a searchable version of the select using an input and a custom menu:

Screenshot 2023-08-29 at 10 37 23 AM

Also includes a bundle of small fixes and adjustments that came up during review/testing.

@DTCurrie DTCurrie self-assigned this Aug 24, 2023
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.

Initial thoughts

packages/core/src/lib/select/select-menu.svelte Outdated Show resolved Hide resolved
packages/core/src/lib/select/select.svelte Outdated Show resolved Hide resolved
packages/core/src/lib/select/utils.ts Outdated Show resolved Hide resolved
packages/core/src/lib/select/utils.ts Outdated Show resolved Hide resolved
packages/core/src/lib/select/utils.ts Outdated Show resolved Hide resolved
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.

I see how the store-based solution for unique IDs works, but I don't love how it requires to add an extra context provider to use the some components in the library. Given that this implementation is not SSR stable, would it be better to simply go with nanoid/non-secure and forgo adding in a required context?

packages/core/src/lib/unique-id.ts Outdated Show resolved Hide resolved
packages/core/src/lib/unique-id.ts Outdated Show resolved Hide resolved
packages/core/src/lib/unique-id.ts Outdated Show resolved Hide resolved
packages/core/src/lib/unique-id.ts Outdated Show resolved Hide resolved
@DTCurrie
Copy link
Member Author

I see how the store-based solution for unique IDs works, but I don't love how it requires to add an extra context provider to use the some components in the library. Given that this implementation is not SSR stable, would it be better to simply go with nanoid/non-secure and forgo adding in a required context?

We could also just have it track the count internally without a store, but then we will need to require it be imported via a context="module" script which could add a level confusion.

I am cool with just using nanoid or just a string generator function in JS if we aren't super worried about it.

@DTCurrie DTCurrie force-pushed the add-searchable-select branch 4 times, most recently from a45cbe4 to d5afeca Compare August 28, 2023 19:54
@DTCurrie DTCurrie marked this pull request as ready for review August 29, 2023 14:36
packages/core/src/lib/pill.svelte Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: consider using userEvent for this testing to simulate interactions

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed we hadn't migrated elsewhere and @testing-library/svelte does not include userEvent. If we want to move to that, I would rather have a follow-up ticket to apply it in all of our tests instead of having a mix of solutions.

Comment on lines +60 to +61
0: SearchMatch[];
'-1': SearchMatch[];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these necessary given L59?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we always set them as present, it makes the typing a lot more clear in the IDE especially since we are constantly checking against 0 and -1. Plus we always want to enforce they are present and arrays:

Screenshot 2023-08-29 at 12 47 02 PM Screenshot 2023-08-29 at 12 46 40 PM

packages/core/src/lib/select/search.ts Outdated Show resolved Hide resolved
packages/core/src/lib/select/searchable-select.svelte Outdated Show resolved Hide resolved
Comment on lines 26 to 29
input: string | undefined;
change: string | undefined;
search: string;
buttonclick: undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the intention of an undefined event value? If undefined is meant to mean "empty", could this be typed as null? Or maybe an explicitly typed object would be most clear to the consumer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It signals the event does not include any data, essentially a () => void handler. We just want to signal when the button has been clicked, we make no other assumptions about it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the cases that I've wanted this, I've explicitly typed the return as Record<string, never> to signal to the consumer the lack of associated detail

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read through a bunch on this and landed on this PR where the feature was added, specifically this comment: sveltejs/svelte#7224 (comment)

Also with this, we signal to the user the detail is undefined:
Screenshot 2023-08-29 at 1 10 28 PM

If I use Record<string, never> this is what we show the user, which is way less clear IMO:
Screenshot 2023-08-29 at 1 11 25 PM

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though looking at the migration docs for v4 it seems null is preferred so we can swap to that. Generally I prefer undefined but I don't mind sticking to their recommendation: https://svelte.dev/docs/v4-migration-guide#stricter-types-for-svelte-functions

packages/core/src/lib/select/select.svelte Outdated Show resolved Hide resolved
packages/core/src/routes/+page.svelte Outdated Show resolved Hide resolved
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.

LGTM, agree with Ethan's comments

packages/core/plugins.ts Outdated Show resolved Hide resolved
packages/core/src/lib/select/search.ts Outdated Show resolved Hide resolved
@DTCurrie DTCurrie merged commit fd61b89 into viamrobotics:main Aug 29, 2023
5 checks passed
@DTCurrie DTCurrie deleted the add-searchable-select branch August 29, 2023 21:26
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.

4 participants