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

Improve search functionality in SelectField and MultiSelect #577

Merged

Conversation

brandonmcconnell
Copy link
Contributor

This change adds for overriding searchable text in SelectField and MultiSelect components with new searchLabel property on the MenuOption type

Copy link

stackblitz bot commented Mar 13, 2025

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

changeset-bot bot commented Mar 13, 2025

🦋 Changeset detected

Latest commit: 1b17d69

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte-ux Patch

Not sure what this means? Click here to learn what changesets are.

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

@brandonmcconnell brandonmcconnell changed the title Add support for optional searchLabel property in SelectField Add support for optional searchLabel property in SelectField and MultiSelect Mar 13, 2025
Copy link
Contributor

github-actions bot commented Mar 13, 2025

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

Name Status Preview Last Commit
svelte-ux ✅ Ready (View Log) Visit Preview 1b17d69

Copy link

pkg-pr-new bot commented Mar 13, 2025

Open in Stackblitz

npm i https://pkg.pr.new/svelte-ux@577

commit: 1b17d69

@techniq
Copy link
Owner

techniq commented Mar 13, 2025

I'm not sure it's worth increasing the API surface area, but it looks like we need to refine the contract of <SelectField search={...}> from...

export let search = async (text: string) => {
  logger.debug('search', { text, open });

  if (text === '') {
    // Reset options
    filteredOptions = options;
  } else {
    const words = text?.toLowerCase().split(' ') ?? [];
    filteredOptions = options.filter((option) => {
      return words.every((word) => option.label.toLowerCase().includes(word));
    });
  }
};

to instead return the filtered options...

export let search = async (text: string) => {
  logger.debug('search', { text, open });

  if (text === '') {
    // Reset options
    return options;
  } else {
    const words = text?.toLowerCase().split(' ') ?? [];
    return options.filter((option) => {
      return words.every((word) => option.label.toLowerCase().includes(word));
    });
  }
};

and then change where we are handling the results to apply them from...

search(searchText).then(() => {
  ...
})

to...

search(searchText).then((options) => {
  filteredOptions = options;
  ...
})

This should give you the flexibility you need. I thought if it would be worth passing options into search as well async (text, options) but within search you might be making a fetch call to retrieve the options from a server. If it's using local state for options, it would be in scope as well, and you could create a function that takes in options and returns the search function (currying).

Note: you should be able to accomplish this now by tracking options externally and updating it, which is probably what you are using now as a solution (per discord discussion).

@brandonmcconnell
Copy link
Contributor Author

@techniq Yes, I went the route of tracking the three search texts and three filtered objects outside of the MultiSelect, and then simplified the usage of search—currently—to only manage the search term, which then reactively filters the results for those three arrays of objects.

The idea you mentioned above would definitely be a better DX than the current search, though it might be considered a breaking change (not sure how many people use that). This PR's feature would still be more straightforward for simpler use cases like those I've mentioned, but I can see how this might raise other questions about case sensitivity, etc. You could also add a caseSensitive prop and call it a day, but I'm not sure how far down that rabbit hole goes and if you'd eventually need to introduce some options object to manage all of that.

What you've proposed seems like a big and flexible win, requiring only a minimal amount of work (this way, we could avoid maintaining the search state and separate filtered objects outside the component usage).

Thanks!

@techniq
Copy link
Owner

techniq commented Mar 14, 2025

@techniq Yes, I went the route of tracking the three search texts and three filtered objects outside of the MultiSelect, and then simplified the usage of search—currently—to only manage the search term, which then reactively filters the results for those three arrays of objects.

The idea you mentioned above would definitely be a better DX than the current search, though it might be considered a breaking change (not sure how many people use that). This PR's feature would still be more straightforward for simpler use cases like those I've mentioned, but I can see how this might raise other questions about case sensitivity, etc. You could also add a caseSensitive prop and call it a day, but I'm not sure how far down that rabbit hole goes and if you'd eventually need to introduce some options object to manage all of that.

What you've proposed seems like a big and flexible win, requiring only a minimal amount of work (this way, we could avoid maintaining the search state and separate filtered objects outside the component usage).

Thanks!

I think this change can be considered a fix: the current state is a bit broken for override use cases, but we can also apply the change against the next branch which is released as 2.0.0-next.#. Note, this branch is now on Tailwind 4 and will soon require Svelte 5.

You might also be able to change this while maintaining full backwards compatible by checking if search returned any values, and if not, fallback to all options (which should have been the result of overriding search with the current API).

search(searchText).then((_options) => {
  filteredOptions = _options ?? options;
  ...
})

@brandonmcconnell
Copy link
Contributor Author

brandonmcconnell commented Mar 14, 2025

@techniq I'm currently working to get us on the latest versions of svelte-ux and layerstack. It would be great if we could get this fix (or similar) into the most recent non-svelte5/tw4 version, as it'll likely take us longer to upgrade both svelte and tailwind.

The current API for this change would be something like this:

<SelectField
  options={users.map((u) => ({
    ...o,
    value: u.id,
    label: u.name,
    searchLabel: [u.name, u.email]
  }))}
>
  <svelte:fragment slot="option" let:option={user}>
    <MenuItem>
      <div data-id={user.id}>{user.name} ({user.email})</div>
    </MenuItem>
  </svelte:fragment>
</SelectField>
/>

I don't like that my initial solution here requires a property to be added on the MenuOption type, since this exists as a filter to iterate over the array. Feels funky to put the filter condition in the array elements themselves.

I wonder if there's a better fix that would exist as a hybrid between this and the existing search function, something that operates similarly to Array.prototype.filter, takes a simple optionally asynchronous callback to test each option when searching, and then returns a boolean of whether or not it matches. Something like this:

<SelectField
  options={users.map((u) => ({
    ...o,
    value: u.id,
    label: u.name,
  }))}
  search={async (user, searchText) => {
    const words = searchText?.toLowerCase().split(' ') ?? [];
    const label = [user.name, user.email].filter(Boolean).join(' ');
    return words.every((word) => label.toLowerCase().includes(word));
  }}
>
  <svelte:fragment slot="option" let:option={user}>
    <MenuItem>
      <div data-id={user.id}>{user.name} ({user.email})</div>
    </MenuItem>
  </svelte:fragment>
</SelectField>
/>

This does feel much cleaner in my opinion. I think this could be a good fix to the search prop. I think it could also be worthwhile to add a helper function to abstract away this pattern of breaking up the words since it's a very common pattern, both used in the search prop fallback value (defined in the SelectField component) and the pattern I've needed to manually implement for that search prop whenever I need to expose additional values to include in the search.

Something like this could work:

<SelectField
  options={users.map((u) => ({
    ...o,
    value: u.id,
    label: u.name,
  }))}
  search={searchFor((user) => [user.name, user.email])}
>
  <svelte:fragment slot="option" let:option={user}>
    <MenuItem>
      <div data-id={user.id}>{user.name} ({user.email})</div>
    </MenuItem>
  </svelte:fragment>
</SelectField>
/>

In this case, the searchFor function would be defined as follows:

<!-- SelectField.svelte -->
<script lang="ts" context="module">
  export function searchFor<T extends unknown>(fn: (option: T) => string | string[]) {
    return async (option: T, searchText: string) => {
    const words = searchText?.toLowerCase().split(' ') ?? [];
    const searchData = fn(option);
    const label = String(
      Array.isArray(searchData)
        ? searchData.filter(Boolean).join(' ')
        : searchData
    ).trim().toLowerCase();
    return words.every((word) => label.includes(word));
  }
<!-- SelectField_Consumer.svelte -->
<script lang="ts">
  import { default as SelectField, searchFor } from './SelectField.svelte';
</script>

Although, those imports might look prettier if you move that into a util somewhere. After all, that pattern is common and could be useful in plenty of other search or filtering contexts.

@techniq
Copy link
Owner

techniq commented Mar 14, 2025

I think the best solution is for search to handle all options and not on a per-option/record basis as this wouldn't scale for API requests and could be less performant depending on how the filtering in implemented (might be using fuse.js, etc)

For your example it would look like:

<SelectField
  options={users.map((u) => ({
    ...o,
    value: u.id,
    label: u.name,
  }))}
  search={async (searchText) => {
    const words = searchText?.toLowerCase().split(' ') ?? [];
    return users.filter(user => {
      const label = [user.name, user.email].filter(Boolean).join(' ');
      return words.every((word) => label.toLowerCase().includes(word));
    }
  }}
>
  <svelte:fragment slot="option" let:option={user}>
    <MenuItem>
      <div data-id={user.id}>{user.name} ({user.email})</div>
    </MenuItem>
  </svelte:fragment>
</SelectField>
/>

and an API search could look like

<SelectField
  options={users.map((u) => ({
    ...o,
    value: u.id,
    label: u.name,
  }))}
  search={async (searchText) => {
    // Example API call but can be structured many ways
    const results = await api('/search', { searchText });
    // You probably need to `results.map(...)` to `MenuOption` `{ label, value }` depending on the response
    return results;
  }}
>
  <svelte:fragment slot="option" let:option={user}>
    <MenuItem>
      <div data-id={user.id}>{user.name} ({user.email})</div>
    </MenuItem>
  </svelte:fragment>
</SelectField>
/>

@brandonmcconnell
Copy link
Contributor Author

brandonmcconnell commented Mar 14, 2025

@techniq I think that makes a lot of sense. I think it could be useful to include the current list of options as the second arg in that callback, for the same reason that most Array.prototype methods expose the source array as the final callback parameter.

This way, if the options array is constructed inline (as shown in my users example above), the final constructed object would also be available for use as the second parameter.

What did you think about my idea for abstracting away that common search-filter pattern to a helper function util?

That would look like this:

<SelectField
  options={users.map((u) => ({
    ...o,
    value: u.id,
    label: u.name,
  }))}
  search={searchFor((user) => [user.name, user.email])}
  search={async (searchText, users) => {
    const words = searchText?.toLowerCase().split(' ') ?? [];

    /* Option 1️⃣: `searchFor` returns the filter callback */
    return users.filter(searchFor((user) => [user.name, user.email]));
    /* Option 2️⃣: `searchFor` explicitly works inside the filter callback */
    return users.filter((user) => searchFor([user.name, user.email]));
  }}
>
  <svelte:fragment slot="option" let:option={user}>
    <MenuItem>
      <div data-id={user.id}>{user.name} ({user.email})</div>
    </MenuItem>
  </svelte:fragment>
</SelectField>

I think could that whichever one of these two options yields the most reusable value could be worth including in utils, even in @layerstack/utils for utilization in cases like this.

@techniq
Copy link
Owner

techniq commented Mar 14, 2025

@techniq I think that makes a lot of sense. I think it could be useful to include the current list of options as the second arg in that callback, for the same reason that most Array.prototype methods expose the source array as the final callback parameter.

This way, if the options array is constructed inline (as shown in my users example above), the final constructed object would also be available for use as the second parameter.

What did you think about my idea for abstracting away that common search-filter pattern to a helper function util?

That would look like this:

<SelectField
  options={users.map((u) => ({
    ...o,
    value: u.id,
    label: u.name,
  }))}
  search={searchFor((user) => [user.name, user.email])}
  search={async (searchText, users) => {
    const words = searchText?.toLowerCase().split(' ') ?? [];

    /* Option 1️⃣: `searchFor` returns the filter callback */
    return users.filter(searchFor((user) => [user.name, user.email]));
    /* Option 2️⃣: `searchFor` explicitly works inside the filter callback */
    return users.filter((user) => searchFor([user.name, user.email]));
  }}
>
  <svelte:fragment slot="option" let:option={user}>
    <MenuItem>
      <div data-id={user.id}>{user.name} ({user.email})</div>
    </MenuItem>
  </svelte:fragment>
</SelectField>

I think could that whichever one of these two options yields the most reusable value could be worth including in utils, even in @layerstack/utils for utilization in cases like this.

Passing options as the second arg makes sense, but searchFor should be a user concern for now.

@brandonmcconnell
Copy link
Contributor Author

I dig it. I'll update my PR per that spec.

- update `search` prop logic
- remove `searchLabel`
- rename `inlineSearch` -> `search`
- add support in `MultiSelect` for passing custom search function into `search` prop
@brandonmcconnell
Copy link
Contributor Author

brandonmcconnell commented Mar 15, 2025

Made some updates to align with that new vision

  • update search prop logic
  • remove searchLabel
  • update search functionality in MultiSelect
    Their underlying logic was nearly identical, so I made these changes to unify their usage
    • rename inlineSearch -> search 🚩
    • add support for passing custom search function into search prop

@brandonmcconnell brandonmcconnell changed the title Add support for optional searchLabel property in SelectField and MultiSelect Improve search functionality in SelectField and MultiSelect Mar 15, 2025
Copy link
Owner

@techniq techniq left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @brandonmcconnell!

@techniq techniq merged commit 7ad0cf1 into techniq:main Mar 18, 2025
4 checks passed
@brandonmcconnell
Copy link
Contributor Author

Thanks, @techniq!

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