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

fix: select component to handle empty string values for keyboard #979

Merged
merged 7 commits into from
Dec 6, 2024

Conversation

max-got
Copy link

@max-got max-got commented Dec 6, 2024

Closes #977

Fixed empty value keyboard selection for SelectComponent.
The diff is very large, but just because i've added an empty value option in the items array inpackages/tests/src/tests/select/select.test.ts file.

Added a few tests as well + enhanced the Components for the Docs to showcase disabled options and an option with an empty ("") value. Tho that can be further enhanced, to showcase the possibilities even further.

I have added test helpers in packages/tests/src/tests/helpers/select.ts because i needed a way to generate a valid testId for the select components. Because there are 3 Components + the test file, it felt right to abstract them away in a helper. I dont't know if that this is your desired approach. Let me know if there is something wrong.

…ard navigation

- Updated `SelectTriggerState` to correctly handle `highlightedValue` as a valid empty string.
- Introduced `generateTestId` and `getTestId` functions to ensure consistent test ids for select items, accommodating empty strings and null values.
- Modified multiple test files to utilize the new test id generation logic.
- Updated demo components to include options with empty string values and disabled attribute
Copy link

changeset-bot bot commented Dec 6, 2024

🦋 Changeset detected

Latest commit: 221b20e

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

This PR includes changesets to release 1 package
Name Type
bits-ui 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

Copy link
Contributor

github-actions bot commented Dec 6, 2024

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

Name Status Preview Last Commit
bits-ui ✅ Ready (View Log) Visit Preview 221b20e

@huntabyte
Copy link
Owner

Hey @max-got thanks a ton for digging into this.

I think it's a bad practice to have an empty string as a value of one of the options as it makes it indistinguishable from an item not being selected at all. When you open the select, it has a value selected, but the placeholder is shown, and the data-placeholder styles are applied.

I'm fine with this being added, but we won't be demonstrating it in the main demo or showing any examples of it. It will be more of a case of if you really want to do this, it's possible, but there are more explicit ways it should probably be done (i.e. using allowDeselect) and not having a value selected at all. If none is truly an explicit "theme" the user can select, it should be tied to a value that isn't an empty string (IMO of course).

@max-got
Copy link
Author

max-got commented Dec 6, 2024

Totally fair point. I read up a bit on value="" and unfortunately didn't get a definitive answer as to which is better. Especially in relation to accessibility concerns. With the change I just wanted to bring the Select component closer to the native <select> element. The ARIA (APG) Select-Only Combobox Example also sets an initial "option" which is value-less. The difference is that this example uses the first option as placeholder tho. So i'am agreeing with:

When you open the select, it has a value selected, but the placeholder is shown, and the data-placeholder styles are applied.

And to:

[...] but we won't be demonstrating it in the main demo or showing any examples of it [...] 👍

Maybe we can think about it a bit? I will revert the demo example

For example the following is closer to the <select>, which imo some consumer prefer.

const themes = [
+		{ value: "", label: "Select a theme" },
		{ value: "light-monochrome", label: "Light Monochrome" },
		{ value: "dark-green", label: "Dark Green" },
		//...
	];

	let value = $state<string>("");
	const selectedLabel = $derived(
- 		value ? themes.find((theme) => theme.value === value)?.label : "Select a Theme"
+		value ? themes.find((theme) => theme.value === value)?.label : themes[0].label
	);

but the bottom line is, i don't know either 🤷‍♂️

@huntabyte
Copy link
Owner

Yeah, I see their example does do that... interesting 🤔

For now, let's leave it off our main example here, but I'll take it under consideration as the "default" experience.

If you think it would be beneficial, we could add another example towards the bottom in its own docs section showcasing that capability!

@max-got
Copy link
Author

max-got commented Dec 6, 2024

I don't think there is a need for extra docs/section. There will be issues/questions if something is off

Copy link
Owner

@huntabyte huntabyte left a comment

Choose a reason for hiding this comment

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

Thank you!

@huntabyte huntabyte merged commit a5e376d into huntabyte:next Dec 6, 2024
5 checks passed
@github-actions github-actions bot mentioned this pull request Dec 6, 2024
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