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

Select: Rework? #235

Closed
lukeed opened this issue Dec 14, 2023 · 9 comments · May be fixed by #469
Closed

Select: Rework? #235

lukeed opened this issue Dec 14, 2023 · 9 comments · May be fixed by #469
Labels
Svelte 5 Roadmap This issue is planned to be addressed or fixed when the library migrates to Svelte 5.

Comments

@lukeed
Copy link

lukeed commented Dec 14, 2023

The selected approach is awkward, tbh. In practice, it prevents users from binding to a real value. Instead, theyre forced to construct a selected and then use onSelectedChange to manually update the original value + the selected {value,label}

IMO something like the below should be offered as default implementation. Would PR but it breaks away from the standard export signature of the rest of the components; eg export { Root as Select } would likely be replaced by export { Below as Select } which then means the <X.Root/> === <X> aliasing pattern is broken.

<script lang="ts" context="module">
  export type Option<T=string|number> = {
    value: T;
    label: string;
  };
</script>

<script lang="ts">
  import { Select as Primitive } from 'bits-ui';
  import type { HTMLButtonAttributes } from 'svelte/elements';
  import * as Select from '.';

  import { find } from '@/utils';

  type T = $$Generic<string | number>;

  type $$Props = HTMLButtonAttributes & {
    open?: boolean;
    disabled?: boolean;
    required?: boolean;
    id?: string;
    name?: string;
    value?: T | null;
    placeholder?: string;
    options: Option<T>[];
  }

  export let open = false;
  export let disabled = false;
  export let required = false;

  export let id: string | undefined = undefined;
  export let name: string | undefined = undefined;

  export let options: Option<T>[];
  export let value: T | null | undefined = undefined;
  export let placeholder = 'Select option';

  $: selected = value != null ? find(options, x => x.value === value) : undefined;
  $: display = selected ? selected.label : placeholder;

  function onChange(option: unknown) {
    let o = option as Option<T>;
    if (o) value = o.value;
  }
</script>

<Primitive.Root
  loop bind:open
  {disabled} {required} {name}
  {selected} onSelectedChange={onChange}
>
  <Select.Trigger {id} {...$$restProps}>
    <Select.Value
      placeholder={display}
      class={ !value && "text-muted-foreground" || "" }
    />
  </Select.Trigger>

  <Select.Input />

  <Select.Content class="p-0 w-[400px] max-h-96 overflow-y-auto overscroll-y-contain">
    {#each options as o (o.value)}
      <Select.Item value={o.value}>{o.label}</Select.Item>
    {/each}
  </Select.Content>
</Primitive.Root>

Either the Select.Content or the Select.Item loop could be slotted w/ the above parts as default markup

@huntabyte
Copy link
Owner

Hey @lukeed, I agree with your feelings on the selected approach. The reason it was introduced in the first place to Melt UI was that users were programmatically updating the value and the label wasn't reflecting that, but I want to say it was a limitation with actions.

I'd like to address it here in bits, potentially as a separate implementation that wouldn't depend on Melt, but I haven't had the time to figure out the ideal approach that checks the boxes:

  • properly typed values (for the change function/binding)
  • allows for multiple selection
  • keeps the label in sync with the value (should it be programmatically changed)

Looking at other headless libraries like Radix, it appears they only accept string values which certainly simplifies things, but puts more onto the user should they want to bind to a non primitive value.

I don't think this is a terrible idea for the shadcn-svelte implementation, but as you mentioned it deviates from the rest of the components. I'll need to think on this one for a bit - but a possible middle-ground for the time being could be adding something to the docs with this snippet to demonstrate creating reusable selects with limited value types.

@lukeed
Copy link
Author

lukeed commented Dec 15, 2023

Agreed. Although numbers should be allowed because the user can bind:value={pageSize} on this select & still have it reflected as a number.

I've updated my original code snippet w/ my latest draft which includes generic T for value and options type matching.
Label is already kept in sync & personally I think it only makes sense to customize the empty placeholder state. The place for customizing the labels is within Option['label']

I personally have a different UI primitive for multiple selection, so I don't have it worked into here although it certainly could be. The only part that really changes is how one displays the selected value(s).

@drahmedshaheen
Copy link
Contributor

It's worth noting that wrapping values with the Selected type is a great idea. While it may not be apparent with strings, for nested objects, it eliminates the need to map again and again to return the chosen value.

import { Select, type Selected } from 'bits-ui';

  let selectedPhoneNumber: Selected<PhoneNumber>;
	$: selectedPhoneNumber = {
		value: customer?.phoneNumber ?? initialValue.customer.phoneNumber,
		label: customer?.phoneNumber?.value ?? initialValue.customer.phoneNumber.value,
	};

	let selectedAddress: Selected<Address>;
	$: selectedAddress = {
		value: customer?.address! ?? initialValue.customer.address,
		label: customer?.address!.formattedValue ?? initialValue.customer.address!.formattedValue,
	};

	function selectPhoneNumber({ value }: Selected<PhoneNumber>) {
		customer.phoneNumber = value;
	}

	function selectAddress({ value }: Selected<PhoneNumber>) {
		customer.address = value;
	}
	
<Select.Root selected={selectedPhoneNumber} onSelectedChange={selectPhoneNumber}>

while using a context store to save these data for another component

I assume that Svelte 5 will remove many complications, which might make things more reactive and neglect that bridge.

Finally, I would like to thank you for sharing my thoughts

@lukeed
Copy link
Author

lukeed commented Dec 17, 2023

@Ashaheen92 Look how much more you have to type. And you'd have to repeat that per Select usage. If you were able to bind to a value with a structured list of Options, then all the label/display matching would be automated.

@drahmedshaheen
Copy link
Contributor

@lukeed, you're absolutely right, and I acknowledge that you have more expertise than I do. I'm still in the learning process, but based on my considerations, using this approach seems to be the best practice. There are situations where you have no control over the data value you need or the label you want to display.

I'm not trying to argue with you; I just want to share my perspective based on the specific use case. In my opinion, it's often more beneficial to enhance the existing solution rather than completely changing it. For instance, introducing a function named toSelected could facilitate handling various value types throughout the entire component.

I appreciate your concern regarding my comment. Thank you for engaging me in this discussion.

@huntabyte
Copy link
Owner

huntabyte commented Feb 14, 2024

Alright, I've had some time to think about this one a bit more.

How would you all feel about requiring the items as a prop to the Select.Root?

Here's how it may work:

type SelectProps<T, U> = {
	items: T[];
	toValue: (item: T) => U;
	toLabel: (item: T) => string;
	// ... other props
}

This way you can pass any arbitrary array of data, which could be an array of primitive values, objects, arrays, or whatever. You just need to let us know how to convert those into values/labels. Then we could use a Map to link the value with its label, so you could programmatically update the value and the label would react as well.

<script lang="ts">
	import { Select } from 'bits-ui'

	const myItems = [
		{ id: 123, age: 28, name: { first: 'hunter', last: 'johnston' }, address: '123 main st.' },
		{ id: 345, age: 32, name: { first: 'mike', last: 'smith' }, address: '331 bird st.' },
		// ...
	]
</script>

<Select.Root items={myItems} toValue={(item) => item.id} toLabel={(item) => item.name.first + ' ' + item.name.last}>
	<Select.Trigger />
	<Select.Content let:items>
		{#each items as item}
			<Select.Item {item} />
		{/each}
	</Select.Content>
</Select.Root>

or

<script lang="ts">
	import { Select } from 'bits-ui'

	const myItems = ['apple', 'orange', 'mango']
</script>

<Select.Root items={myItems} toValue={(item) => item} toLabel={(item) => item}>
	<Select.Trigger />
	<Select.Content let:items>
		{#each items as item}
			<Select.Item {item} />
		{/each}
	</Select.Content>
</Select.Root>

This would mean that value would simply represent the value rather than an object containing both value and label. The only way I see this workout without this way is if we only allow value to be a string, which I imagine is not ideal for a lot of peeps.

@drahmedshaheen
Copy link
Contributor

Yes, It is better to represent the value rather than an object containing both value and label but toValue and toLabel could add unnecessary complexity and overload over the root component. Before engaging with the code, my initial suggestion would have been this approach. However, upon closer examination, I now recommend utilizing the label and value attributes of Select.Item to maintain simplicity.

<script lang="ts">
	import { Select } from 'bits-ui'

	const myItems = [
		{ id: 123, age: 28, name: { first: 'hunter', last: 'johnston' }, address: '123 main st.' },
		{ id: 345, age: 32, name: { first: 'mike', last: 'smith' }, address: '331 bird st.' },
		// ...
	]

	let value = undefiend
</script>

<Select.Root items={myItems} bind:value>
	<Select.Trigger />
	<Select.Content let:items>
		{#each items as item}
			<Select.Item {item}
  			value={item.id}
  			label={item.name.first + ' ' + item.name.last}/>
		{/each}
	</Select.Content>
</Select.Root>

In addition, I propose consistently displaying the label, eliminating the need for duplication when providing ItemIndicator, which will replace the default slot and remain visible in Select.Value
Instead of:

<slot {builder} {isSelected}>
	{label ? label : value}
</slot>

Consider:

<span> {label ? label : value} </span>
<slot {builder} {isSelected} />

Furthermore, I advocate for adhering to standardized attributes and markdown naming for HTML elements, aligning with MDN select. For instance, preferring Select.Option over Select.Item.

These are suggestions I offer for your consideration. Should you find them worthwhile, I am eager to provide a PR to implement them.

@huntabyte
Copy link
Owner

huntabyte commented Feb 15, 2024

The problem with using the value and label of the Select.Item is that technically those aren't mounted until you open the select (since they are children of the Content), so there would be no way of registering the value -> label mapping beforehand, otherwise, that'd be the approach I'd take.

If a user had never opened the select or conditionally rendered some (imagine a virtual list of sorts), and you had something else that programmatically updated the value, there would be no mapping and it would fall on its face.

The label -> value mapping has to happen at the root otherwise there are a number of issues that arise. We exhausted this with Melt before saying screw it and going to the selected object to guarantee there is always a value and label.

@huntabyte huntabyte mentioned this issue Jun 9, 2024
3 tasks
@huntabyte huntabyte mentioned this issue Jul 31, 2024
70 tasks
@huntabyte huntabyte added the Svelte 5 Roadmap This issue is planned to be addressed or fixed when the library migrates to Svelte 5. label Sep 19, 2024
@huntabyte
Copy link
Owner

Closing as addressed in bits-ui@next

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Svelte 5 Roadmap This issue is planned to be addressed or fixed when the library migrates to Svelte 5.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants