-
Notifications
You must be signed in to change notification settings - Fork 12
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
Allow ReactElements to be used in Combobox dropdowns #2474
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
The behavior where it automatically focuses the image field on error is broken — In theory, diff --git a/app/ui/lib/Combobox.tsx b/app/ui/lib/Combobox.tsx
index 946cd236..0dbfa2d9 100644
--- a/app/ui/lib/Combobox.tsx
+++ b/app/ui/lib/Combobox.tsx
@@ -15,7 +15,7 @@ import {
} from '@headlessui/react'
import cn from 'classnames'
import { matchSorter } from 'match-sorter'
-import { useId, useState, type ReactNode } from 'react'
+import { useId, useState, type ReactNode, type Ref } from 'react'
import { SelectArrows6Icon } from '@oxide/design-system/icons/react'
@@ -69,6 +69,8 @@ type ComboboxProps = {
selectedItemLabel: string
hasError?: boolean
onChange: (value: string) => void
+ /** Necessary if you want RHF to be able to focus it on error */
+ inputRef?: Ref<HTMLInputElement>
} & ComboboxBaseProps
export const Combobox = ({
@@ -86,6 +88,7 @@ export const Combobox = ({
onInputChange,
allowArbitraryValues = false,
hideOptionalTag,
+ inputRef,
...props
}: ComboboxProps) => {
const [query, setQuery] = useState(selectedItemValue || '')
@@ -147,6 +150,7 @@ export const Combobox = ({
: 'bg-default',
hasError && 'focus-error'
)}
+ ref={inputRef}
/>
{items.length > 0 && (
<ComboboxButton
|
Ok, it's working now. Weird. |
This also closes #2483 |
This morning, talked through an edge case we'd like to polish up in a future release, but don't want to hold up merging this with the current release. I'll file a ticket, but the summary is that when there's a validation error, we can't open up the popup (by giving the input focus) without also obscuring the error message that's highlighting why it's in an error state. Ben had a few ideas that we'll explore a bit in a subsequent branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woohoo!
oxidecomputer/console@073471c...1733b76 * [1733b764](oxidecomputer/console@1733b764) oxidecomputer/console#2474 * [efcaba79](oxidecomputer/console@efcaba79) narrower allow-run on bump-omicron.ts
oxidecomputer/console@073471c...1733b76 * [1733b764](oxidecomputer/console@1733b764) oxidecomputer/console#2474 * [efcaba79](oxidecomputer/console@efcaba79) narrower allow-run on bump-omicron.ts
For #1837, we had a few more places that needed to get switched from a Listbox to a Combobox. Interestingly, all of our existing Comboboxes had had the same string used for both the
label
and thevalue
, so there were a few places where the Combobox was using thelabel
where it would more accurately use thevalue
. As these were identical before, it didn't cause any problems. With this update, though, we'll begin using elements that have a differentvalue
from theirlabel
, so it's more important that we use thevalue
.One example of this is in the image selector, where we now have three elements — the

value
, thelabel
, and an optionalselectedLabel
— a custom string for the ComboboxItem.Here we see the
selectedLabel
in the input field, where thelabel
for each item is the richer HTML in the dropdown.Closes #1837
Closes #2483