From a57a86d66e95e643c9779476ea7f7faff6a4d78e Mon Sep 17 00:00:00 2001 From: Benjamin Leonard Date: Wed, 13 Aug 2025 16:11:16 +0100 Subject: [PATCH 1/4] Text field input cleanup --- app/components/form/fields/TextField.tsx | 59 +++++++----------------- app/forms/firewall-rules-common.tsx | 47 ++++++++----------- app/forms/network-interface-edit.tsx | 58 +++++++++++------------ app/forms/project-create.tsx | 2 +- app/pages/LoginPage.tsx | 36 +++++++-------- 5 files changed, 79 insertions(+), 123 deletions(-) diff --git a/app/components/form/fields/TextField.tsx b/app/components/form/fields/TextField.tsx index 5348b7c273..6732c679a2 100644 --- a/app/components/form/fields/TextField.tsx +++ b/app/components/form/fields/TextField.tsx @@ -5,6 +5,7 @@ * * Copyright Oxide Computer Company */ +import cn from 'classnames' import { useId } from 'react' import { useController, @@ -30,6 +31,7 @@ export interface TextFieldProps< TFieldValues extends FieldValues, TName extends FieldPath, > extends UITextFieldProps { + variant?: 'default' | 'inline' name: TName /** HTML type attribute, defaults to text */ type?: 'text' | 'password' @@ -54,18 +56,27 @@ export function TextField< TFieldValues extends FieldValues, TName extends FieldPath, >({ + variant = 'default', name, + type = 'text', label = capitalize(name), units, description, required, + control, + validate, + transform, ...props }: Omit, 'id'> & UITextAreaProps) { - // id is omitted from props because we generate it here const id = useId() + const { + field: { onChange, ...fieldRest }, + fieldState: { error }, + } = useController({ name, control, rules: { required, validate } }) return ( -
-
+
+ {/* Hiding the label for inline inputs but keeping it available for screen readers */} +
{label} {units && ({units})} @@ -75,54 +86,18 @@ export function TextField< )}
- {/* passing the generated id is very important for a11y */} - -
- ) -} - -/** - * Primarily exists for `TextField`, but we occasionally also need a plain field - * without a label on it. - * - * Note that `id` is an allowed prop, unlike in `TextField`, where it is always - * generated from `name`. This is because we need to pass the generated ID in - * from there to here. For the case where `TextFieldInner` is used - * independently, we also generate an ID for use only if none is passed in. - */ -export const TextFieldInner = < - TFieldValues extends FieldValues, - TName extends FieldPath, ->({ - name, - type = 'text', - label = capitalize(name), - validate, - control, - required, - id: idProp, - transform, - ...props -}: TextFieldProps & UITextAreaProps) => { - const generatedId = useId() - const id = idProp || generatedId - const { - field: { onChange, ...fieldRest }, - fieldState: { error }, - } = useController({ name, control, rules: { required, validate } }) - return ( - <> onChange(transform ? transform(e.target.value) : e.target.value)} {...fieldRest} {...props} /> + {/* todo: inline error message tooltip */} - +
) } diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 65a5e397f7..8be59fbde2 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -30,7 +30,7 @@ import { ListboxField } from '~/components/form/fields/ListboxField' import { NameField, validateName } from '~/components/form/fields/NameField' import { NumberField } from '~/components/form/fields/NumberField' import { RadioField } from '~/components/form/fields/RadioField' -import { TextField, TextFieldInner } from '~/components/form/fields/TextField' +import { TextField } from '~/components/form/fields/TextField' import { useVpcSelector } from '~/hooks/use-params' import { ProtocolCell, @@ -40,11 +40,9 @@ import { import { Badge } from '~/ui/lib/Badge' import { toComboboxItems } from '~/ui/lib/Combobox' import { FormDivider } from '~/ui/lib/Divider' -import { FieldLabel } from '~/ui/lib/FieldLabel' import { Message } from '~/ui/lib/Message' import { ClearAndAddButtons, MiniTable } from '~/ui/lib/MiniTable' import { SideModal } from '~/ui/lib/SideModal' -import { TextInputHint } from '~/ui/lib/TextInput' import { KEYS } from '~/ui/util/keys' import { ALL_ISH } from '~/util/consts' import { validateIp, validateIpNet } from '~/util/ip' @@ -647,32 +645,23 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = />
- {/* We have to blow this up instead of using TextField to get better - text styling on the label */} -
- - Port filters - - - A single destination port (1234) or a range (1234–2345) - - { - if (e.key === KEYS.enter) { - e.preventDefault() // prevent full form submission - submitPortRange(e) - } - }} - validate={(value) => { - if (!parsePortRange(value)) return 'Not a valid port range' - if (ports.value.includes(value.trim())) return 'Port range already added' - }} - /> -
+ { + if (e.key === KEYS.enter) { + e.preventDefault() // prevent full form submission + submitPortRange(e) + } + }} + validate={(value) => { + if (!parsePortRange(value)) return 'Not a valid port range' + if (ports.value.includes(value.trim())) return 'Port range already added' + }} + />
- {/* We have to blow this up instead of using TextField for better layout control of field and ClearAndAddButtons */} -
- - Transit IPs - - - An IP network, like 192.168.0.0/16.{' '} - - Learn more about transit IPs. - - - { - if (e.key === KEYS.enter) { - e.preventDefault() // prevent full form submission - submitTransitIp() - } - }} - validate={(value) => { - const error = validateIpNet(value) - if (error) return error + + An IP network, like 192.168.0.0/16.{' '} + + Learn more about transit IPs. + + + } + control={transitIpsForm.control} + onKeyDown={(e) => { + if (e.key === KEYS.enter) { + e.preventDefault() // prevent full form submission + submitTransitIp() + } + }} + validate={(value) => { + const error = validateIpNet(value) + if (error) return error - if (transitIps.includes(value)) return 'Transit IP already in list' - }} - placeholder="Enter an IP network" - /> -
+ if (transitIps.includes(value)) return 'Transit IP already in list' + }} + placeholder="Enter an IP network" + /> - + ) diff --git a/app/pages/LoginPage.tsx b/app/pages/LoginPage.tsx index 899c8f2531..1a3831d56c 100644 --- a/app/pages/LoginPage.tsx +++ b/app/pages/LoginPage.tsx @@ -11,7 +11,7 @@ import { useNavigate, useSearchParams } from 'react-router' import { useApiMutation, type UsernamePasswordCredentials } from '@oxide/api' -import { TextFieldInner } from '~/components/form/fields/TextField' +import { TextField } from '~/components/form/fields/TextField' import { useSiloSelector } from '~/hooks/use-params' import { addToast } from '~/stores/toast' import { Button } from '~/ui/lib/Button' @@ -58,24 +58,22 @@ export default function LoginPage() { loginPost.mutate({ body, path: { siloName: silo } }) })} > -
- -
-
- -
+ + From 55ef016774fb71bbca647da6e6419be8cf052d37 Mon Sep 17 00:00:00 2001 From: Benjamin Leonard Date: Wed, 13 Aug 2025 16:17:28 +0100 Subject: [PATCH 2/4] Make `TextInputHint` and `TextErrorMessage` generic --- app/components/form/fields/ErrorMessage.tsx | 11 +++++--- app/components/form/fields/FileField.tsx | 5 ++-- app/components/form/fields/NumberField.tsx | 7 +++-- app/components/form/fields/RadioField.tsx | 7 +++-- app/components/form/fields/SshKeysField.tsx | 7 +++-- app/components/form/fields/TextField.tsx | 7 +++-- app/forms/instance-create.tsx | 6 ++--- app/ui/lib/Combobox.tsx | 7 ++--- app/ui/lib/FieldLabel.tsx | 24 ++++++++++++++++- app/ui/lib/Listbox.tsx | 7 ++--- app/ui/lib/TextInput.tsx | 29 --------------------- 11 files changed, 52 insertions(+), 65 deletions(-) diff --git a/app/components/form/fields/ErrorMessage.tsx b/app/components/form/fields/ErrorMessage.tsx index f804f46189..1fbe3606ad 100644 --- a/app/components/form/fields/ErrorMessage.tsx +++ b/app/components/form/fields/ErrorMessage.tsx @@ -5,10 +5,10 @@ * * Copyright Oxide Computer Company */ +import { announce } from '@react-aria/live-announcer' +import { useEffect } from 'react' import type { FieldError } from 'react-hook-form' -import { TextInputError } from '~/ui/lib/TextInput' - type ErrorMessageProps = { error: FieldError | undefined label: string @@ -20,5 +20,10 @@ export function ErrorMessage({ error, label }: ErrorMessageProps) { const message = error.type === 'required' ? `${label} is required` : error.message if (!message) return null - return {message} + return {message} +} + +export const InputError = ({ children }: { children: string }) => { + useEffect(() => announce(children, 'assertive'), [children]) + return
{children}
} diff --git a/app/components/form/fields/FileField.tsx b/app/components/form/fields/FileField.tsx index c1cb0442c2..a191f700d5 100644 --- a/app/components/form/fields/FileField.tsx +++ b/app/components/form/fields/FileField.tsx @@ -12,9 +12,8 @@ import { type FieldValues, } from 'react-hook-form' -import { FieldLabel } from '~/ui/lib/FieldLabel' +import { FieldLabel, InputHint } from '~/ui/lib/FieldLabel' import { FileInput } from '~/ui/lib/FileInput' -import { TextInputHint } from '~/ui/lib/TextInput' import { ErrorMessage } from './ErrorMessage' @@ -51,7 +50,7 @@ export function FileField< {label} - {description && {description}} + {description && {description}}
({units})} {description && ( - + {description} - + )}
{/* passing the generated id is very important for a11y */} diff --git a/app/components/form/fields/RadioField.tsx b/app/components/form/fields/RadioField.tsx index d69a4a5b9c..7e1fb14d4e 100644 --- a/app/components/form/fields/RadioField.tsx +++ b/app/components/form/fields/RadioField.tsx @@ -15,10 +15,9 @@ import { type PathValue, } from 'react-hook-form' -import { FieldLabel } from '~/ui/lib/FieldLabel' +import { FieldLabel, InputHint } from '~/ui/lib/FieldLabel' import { Radio, type RadioProps } from '~/ui/lib/Radio' import { RadioGroup, type RadioGroupProps } from '~/ui/lib/RadioGroup' -import { TextInputHint } from '~/ui/lib/TextInput' import { capitalize } from '~/util/str' export type RadioFieldProps< @@ -74,7 +73,7 @@ export function RadioField< )} {/* TODO: Figure out where this hint field def should live */} - {description && {description}} + {description && {description}}
)} {/* TODO: Figure out where this hint field def should live */} - {description && {description}} + {description && {description}}
SSH keys - + SSH keys can be added and removed in your user settings - +
{keys.length > 0 ? ( <> diff --git a/app/components/form/fields/TextField.tsx b/app/components/form/fields/TextField.tsx index 6732c679a2..3fbc2078a5 100644 --- a/app/components/form/fields/TextField.tsx +++ b/app/components/form/fields/TextField.tsx @@ -16,9 +16,8 @@ import { type Validate, } from 'react-hook-form' -import { FieldLabel } from '~/ui/lib/FieldLabel' +import { FieldLabel, InputHint } from '~/ui/lib/FieldLabel' import { - TextInputHint, TextInput as UITextField, type TextAreaProps as UITextAreaProps, type TextInputBaseProps as UITextFieldProps, @@ -81,9 +80,9 @@ export function TextField< {label} {units && ({units})} {description && ( - + {description} - + )} Hardware - + Pick a pre-configured machine type that offers balanced vCPU and memory for most workloads or create a custom machine. - + {label} - {description && ( - {description} - )} + {description && {description}} )}
) } + +type HintProps = { + // ID required as a reminder to pass aria-describedby on TextField + id: string + children: ReactNode + className?: string +} + +/** + * Pass id here and include that ID in aria-describedby on the TextField + */ +export const InputHint = ({ id, children, className }: HintProps) => ( +
_a]:underline hover:[&_>_a]:text-raise', + className + )} + > + {children} +
+) diff --git a/app/ui/lib/Listbox.tsx b/app/ui/lib/Listbox.tsx index 71aef4a94f..a55a9acbf5 100644 --- a/app/ui/lib/Listbox.tsx +++ b/app/ui/lib/Listbox.tsx @@ -16,10 +16,9 @@ import { useId, type ReactNode, type Ref } from 'react' import { SelectArrows6Icon } from '@oxide/design-system/icons/react' -import { FieldLabel } from './FieldLabel' +import { FieldLabel, InputHint } from './FieldLabel' import { usePopoverZIndex } from './SideModal' import { SpinnerLoader } from './Spinner' -import { TextInputHint } from './TextInput' export type ListboxItem = { value: Value @@ -92,9 +91,7 @@ export const Listbox = ({ > {label} - {description && ( - {description} - )} + {description && {description}}
)} ) } - -type HintProps = { - // ID required as a reminder to pass aria-describedby on TextField - id: string - children: React.ReactNode - className?: string -} - -/** - * Pass id here and include that ID in aria-describedby on the TextField - */ -export const TextInputHint = ({ id, children, className }: HintProps) => ( -
_a]:underline hover:[&_>_a]:text-raise', - className - )} - > - {children} -
-) - -export const TextInputError = ({ children }: { children: string }) => { - useEffect(() => announce(children, 'assertive'), [children]) - return
{children}
-} From abf9619d403dafc4350f5ddd95d36046b585683a Mon Sep 17 00:00:00 2001 From: Benjamin Leonard Date: Wed, 13 Aug 2025 18:08:23 +0100 Subject: [PATCH 3/4] Extend cleanup to other fields --- app/components/form/fields/ComboboxField.tsx | 69 ++++--- app/components/form/fields/FieldWrapper.tsx | 74 ++++++++ app/components/form/fields/ListboxField.tsx | 69 ++++--- app/components/form/fields/NumberField.tsx | 108 +++++------ app/components/form/fields/RadioField.tsx | 186 +++++++++++-------- app/components/form/fields/TextField.tsx | 61 +++--- app/forms/instance-create.tsx | 104 +++++------ app/ui/lib/Combobox.tsx | 33 +--- app/ui/lib/Listbox.tsx | 30 +-- 9 files changed, 410 insertions(+), 324 deletions(-) create mode 100644 app/components/form/fields/FieldWrapper.tsx diff --git a/app/components/form/fields/ComboboxField.tsx b/app/components/form/fields/ComboboxField.tsx index efa0558293..26ea42bb90 100644 --- a/app/components/form/fields/ComboboxField.tsx +++ b/app/components/form/fields/ComboboxField.tsx @@ -23,16 +23,28 @@ import { } from '~/ui/lib/Combobox' import { capitalize } from '~/util/str' -import { ErrorMessage } from './ErrorMessage' +import { FieldWrapper } from './FieldWrapper' export type ComboboxFieldProps< TFieldValues extends FieldValues, TName extends FieldPath, > = { + variant?: 'default' | 'inline' name: TName control: Control onChange?: (value: string | null | undefined) => void validate?: Validate, TFieldValues> + /** Will default to name if not provided */ + label?: string + /** + * Displayed inline as supplementary text to the label. Should + * only be used for text that's necessary context for helping + * complete the input. This will be announced in tandem with the + * label when using a screen reader. + */ + description?: React.ReactNode + required?: boolean + hideOptionalTag?: boolean } & ComboboxBaseProps export function ComboboxField< @@ -40,6 +52,7 @@ export function ComboboxField< TName extends FieldPath, // TODO: constrain TValue to extend string >({ + variant = 'default', control, name, label = capitalize(name), @@ -62,6 +75,7 @@ export function ComboboxField< items, transform, validate, + hideOptionalTag, ...props }: ComboboxFieldProps) { const { field, fieldState } = useController({ @@ -72,28 +86,37 @@ export function ComboboxField< const [selectedItemLabel, setSelectedItemLabel] = useState( getSelectedLabelFromValue(items, field.value || '') ) + return ( -
- { - field.onChange(value) - onChange?.(value) - setSelectedItemLabel(getSelectedLabelFromValue(items, value)) - }} - allowArbitraryValues={allowArbitraryValues} - inputRef={field.ref} - transform={transform} - {...props} - /> - -
+ + {({ id, 'aria-labelledby': ariaLabelledBy }) => ( + { + field.onChange(value) + onChange?.(value) + setSelectedItemLabel(getSelectedLabelFromValue(items, value)) + }} + allowArbitraryValues={allowArbitraryValues} + inputRef={field.ref} + transform={transform} + {...props} + /> + )} + ) } diff --git a/app/components/form/fields/FieldWrapper.tsx b/app/components/form/fields/FieldWrapper.tsx new file mode 100644 index 0000000000..00c4fd1705 --- /dev/null +++ b/app/components/form/fields/FieldWrapper.tsx @@ -0,0 +1,74 @@ +/* + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, you can obtain one at https://mozilla.org/MPL/2.0/. + * + * Copyright Oxide Computer Company + */ +import cn from 'classnames' +import { useId, type ReactNode } from 'react' +import type { FieldError } from 'react-hook-form' + +import { FieldLabel, InputHint } from '~/ui/lib/FieldLabel' + +import { ErrorMessage } from './ErrorMessage' + +export interface FieldWrapperProps { + variant?: 'default' | 'inline' + label: string | ReactNode + /** + * Displayed inline as supplementary text to the label. Should + * only be used for text that's necessary context for helping + * complete the input. This will be announced in tandem with the + * label when using a screen reader. + */ + description?: string | ReactNode + required?: boolean + hideOptionalTag?: boolean + error?: FieldError + className?: string + /** String label for error messages - falls back to extracting text from ReactNode label */ + errorLabel?: string + children: (props: { id: string; 'aria-labelledby': string }) => ReactNode +} + +export function FieldWrapper({ + variant = 'default', + label, + description, + required, + hideOptionalTag, + error, + className, + errorLabel, + children, +}: FieldWrapperProps) { + const id = useId() + const ariaLabelledBy = cn(`${id}-label`, description ? `${id}-help-text` : '') + + return ( +
+ {/* Hiding the label for inline inputs but keeping it available for screen readers */} +
+ + {label} + + {description && ( + + {description} + + )} +
+ {children({ id, 'aria-labelledby': ariaLabelledBy })} + {/* todo: inline error message tooltip */} + +
+ ) +} diff --git a/app/components/form/fields/ListboxField.tsx b/app/components/form/fields/ListboxField.tsx index 445c8e810c..1c73855cbd 100644 --- a/app/components/form/fields/ListboxField.tsx +++ b/app/components/form/fields/ListboxField.tsx @@ -5,7 +5,6 @@ * * Copyright Oxide Computer Company */ -import cn from 'classnames' import { useController, type Control, @@ -16,17 +15,25 @@ import { import { Listbox, type ListboxItem } from '~/ui/lib/Listbox' import { capitalize } from '~/util/str' -import { ErrorMessage } from './ErrorMessage' +import { FieldWrapper } from './FieldWrapper' export type ListboxFieldProps< TFieldValues extends FieldValues, TName extends FieldPath, > = { + variant?: 'default' | 'inline' name: TName placeholder?: string className?: string + /** Will default to name if not provided */ label?: string required?: boolean + /** + * Displayed inline as supplementary text to the label. Should + * only be used for text that's necessary context for helping + * complete the input. This will be announced in tandem with the + * label when using a screen reader. + */ description?: string | React.ReactNode control: Control disabled?: boolean @@ -41,6 +48,7 @@ export function ListboxField< TFieldValues extends FieldValues, TName extends FieldPath, >({ + variant = 'default', items, name, placeholder, @@ -58,30 +66,39 @@ export function ListboxField< // TODO: recreate this logic // validate: (v) => (required && !v ? `${name} is required` : undefined), const { field, fieldState } = useController({ name, control, rules: { required } }) + return ( -
- { - field.onChange(value) - onChange?.(value) - }} - // required to get required error to trigger on blur - // onBlur={field.onBlur} - disabled={disabled} - name={name} - hasError={fieldState.error !== undefined} - isLoading={isLoading} - buttonRef={field.ref} - hideOptionalTag={hideOptionalTag} - /> - -
+ + {({ id, 'aria-labelledby': ariaLabelledBy }) => ( + { + field.onChange(value) + onChange?.(value) + }} + // required to get required error to trigger on blur + // onBlur={field.onBlur} + disabled={disabled} + name={name} + hasError={fieldState.error !== undefined} + isLoading={isLoading} + buttonRef={field.ref} + aria-labelledby={ariaLabelledBy} + /> + )} + ) } diff --git a/app/components/form/fields/NumberField.tsx b/app/components/form/fields/NumberField.tsx index 7eb1c8a307..b26664b0dc 100644 --- a/app/components/form/fields/NumberField.tsx +++ b/app/components/form/fields/NumberField.tsx @@ -5,77 +5,41 @@ * * Copyright Oxide Computer Company */ -import cn from 'classnames' -import { useId } from 'react' import { useController, type FieldPathByValue, type FieldValues } from 'react-hook-form' -import { FieldLabel, InputHint } from '~/ui/lib/FieldLabel' import { NumberInput } from '~/ui/lib/NumberInput' import { capitalize } from '~/util/str' -import { ErrorMessage } from './ErrorMessage' +import { FieldWrapper } from './FieldWrapper' import type { TextFieldProps } from './TextField' -export function NumberField< +export interface NumberFieldProps< TFieldValues extends FieldValues, - // can only be used on fields with number values TName extends FieldPathByValue, ->({ - name, - label = capitalize(name), - units, - description, - required, - ...props -}: Omit, 'id'>) { - // id is omitted from props because we generate it here - const id = useId() - return ( -
-
- - {label} {units && ({units})} - - {description && ( - - {description} - - )} -
- {/* passing the generated id is very important for a11y */} - -
- ) +> extends Omit, 'type' | 'transform'> { + max?: number + min?: number } -/** - * Primarily exists for `NumberField`, but we occasionally also need a plain field - * without a label on it. - * - * Note that `id` is an allowed prop, unlike in `NumberField`, where it is always - * generated from `name`. This is because we need to pass the generated ID in - * from there to here. For the case where `NumberFieldInner` is used - * independently, we also generate an ID for use only if none is passed in. - */ -export const NumberFieldInner = < +export function NumberField< TFieldValues extends FieldValues, TName extends FieldPathByValue, >({ + variant = 'default', name, label = capitalize(name), - validate, - control, + units, + description, required, - id: idProp, + control, + validate, disabled, max, min = 0, -}: TextFieldProps) => { - const generatedId = useId() - const id = idProp || generatedId - + className, +}: Omit, 'id'>) { const { - field, + field: { onChange, onBlur, value, ref }, fieldState: { error }, } = useController({ name, @@ -90,19 +54,39 @@ export const NumberFieldInner = < }, }) - return ( + const labelWithUnits = units ? ( <> - - + {label} ({units}) + ) : ( + label + ) + + return ( + + {({ id, 'aria-labelledby': ariaLabelledBy }) => ( + + )} + ) } diff --git a/app/components/form/fields/RadioField.tsx b/app/components/form/fields/RadioField.tsx index 7e1fb14d4e..7a33521d81 100644 --- a/app/components/form/fields/RadioField.tsx +++ b/app/components/form/fields/RadioField.tsx @@ -5,8 +5,7 @@ * * Copyright Oxide Computer Company */ -import cn from 'classnames' -import { useId } from 'react' + import { useController, type Control, @@ -15,15 +14,20 @@ import { type PathValue, } from 'react-hook-form' -import { FieldLabel, InputHint } from '~/ui/lib/FieldLabel' import { Radio, type RadioProps } from '~/ui/lib/Radio' import { RadioGroup, type RadioGroupProps } from '~/ui/lib/RadioGroup' import { capitalize } from '~/util/str' -export type RadioFieldProps< +import { FieldWrapper } from './FieldWrapper' + +type RadioElt = React.ReactElement + +// Base props shared by both usage patterns +type RadioFieldBaseProps< TFieldValues extends FieldValues, TName extends FieldPath, > = Omit & { + variant?: 'default' | 'inline' name: TName /** Will default to name if not provided */ label?: string @@ -36,9 +40,20 @@ export type RadioFieldProps< description?: string | React.ReactNode placeholder?: string units?: string + required?: boolean + hideOptionalTag?: boolean control: Control +} + +// Items-based pattern with type safety +type RadioFieldItemsProps< + TFieldValues extends FieldValues, + TName extends FieldPath, +> = RadioFieldBaseProps & { + /** Use items for auto-generated Radio children with type safety */ items: { value: PathValue; label: string }[] -} & (PathValue extends string // this is wild lmao + children?: never +} & (PathValue extends string ? { parseValue?: never } : { /** @@ -49,102 +64,109 @@ export type RadioFieldProps< parseValue: (str: string) => PathValue }) -export function RadioField< +// Children-based pattern for custom components +type RadioFieldChildrenProps< TFieldValues extends FieldValues, TName extends FieldPath, ->({ - name, - label = capitalize(name), - description, - units, - control, - items, - parseValue, - ...props -}: RadioFieldProps) { - const id = useId() - const { field } = useController({ name, control }) - return ( -
-
- {label && ( - - {label} {units && ({units})} - - )} - {/* TODO: Figure out where this hint field def should live */} - {description && {description}} -
- - parseValue ? field.onChange(parseValue(e.target.value)) : field.onChange(e) - } - name={field.name} - {...props} - // TODO: once we get rid of the other use of RadioGroup, change RadioGroup - // to take the list of items too - > - {items.map(({ value, label }) => ( - - {label} - - ))} - -
- ) +> = RadioFieldBaseProps & { + /** Use children for custom Radio components */ + children: RadioElt | RadioElt[] + items?: never + parseValue?: never } -type RadioElt = React.ReactElement - -export type RadioFieldDynProps< +// Union of both patterns +export type RadioFieldProps< TFieldValues extends FieldValues, TName extends FieldPath, -> = Omit, 'parseValue' | 'items'> & { - children: RadioElt | RadioElt[] -} +> = RadioFieldItemsProps | RadioFieldChildrenProps /** - * Same as RadioField, except instead of a statically typed `items` prop, we use - * children to render arbitrary Radio components and therefore cannot guarantee - * anything about the `value` attrs on the radios. This is needed for radio - * cards like the image picker on instance create. + * A flexible radio field component that supports two usage patterns: + * + * 1. Items-based (recommended for simple cases): + * ```tsx + * + * ``` + * + * 2. Children-based (for custom Radio components like radio cards): + * ```tsx + * + * + *
4 vCPUs
+ *
16 GiB RAM
+ * + * + *
16 vCPUs
+ *
64 GiB RAM
+ * + * + * ``` */ -export function RadioFieldDyn< +export function RadioField< TFieldValues extends FieldValues, TName extends FieldPath, >({ + variant = 'default', name, label = capitalize(name), description, units, + required, + hideOptionalTag, control, + items, children, + parseValue, ...props -}: RadioFieldDynProps) { - const id = useId() - const { field } = useController({ name, control }) +}: RadioFieldProps) { + const { field, fieldState } = useController({ name, control, rules: { required } }) + + const labelWithUnits = units ? ( + <> + {label} ({units}) + + ) : ( + label + ) + return ( -
-
- {label && ( - - {label} {units && ({units})} - - )} - {/* TODO: Figure out where this hint field def should live */} - {description && {description}} -
- - {children} - -
+ + {({ 'aria-labelledby': ariaLabelledBy }) => ( + field.onChange(parseValue(e.target.value)) + : field.onChange + } + name={field.name} + {...props} + > + {children || + items?.map(({ value, label }) => ( + + {label} + + ))} + + )} + ) } diff --git a/app/components/form/fields/TextField.tsx b/app/components/form/fields/TextField.tsx index 3fbc2078a5..c5ab0af830 100644 --- a/app/components/form/fields/TextField.tsx +++ b/app/components/form/fields/TextField.tsx @@ -5,8 +5,6 @@ * * Copyright Oxide Computer Company */ -import cn from 'classnames' -import { useId } from 'react' import { useController, type Control, @@ -16,7 +14,6 @@ import { type Validate, } from 'react-hook-form' -import { FieldLabel, InputHint } from '~/ui/lib/FieldLabel' import { TextInput as UITextField, type TextAreaProps as UITextAreaProps, @@ -24,7 +21,7 @@ import { } from '~/ui/lib/TextInput' import { capitalize } from '~/util/str' -import { ErrorMessage } from './ErrorMessage' +import { FieldWrapper } from './FieldWrapper' export interface TextFieldProps< TFieldValues extends FieldValues, @@ -67,36 +64,40 @@ export function TextField< transform, ...props }: Omit, 'id'> & UITextAreaProps) { - const id = useId() const { field: { onChange, ...fieldRest }, fieldState: { error }, } = useController({ name, control, rules: { required, validate } }) + + const labelWithUnits = units ? ( + <> + {label} ({units}) + + ) : ( + label + ) + return ( -
- {/* Hiding the label for inline inputs but keeping it available for screen readers */} -
- - {label} {units && ({units})} - - {description && ( - - {description} - - )} -
- onChange(transform ? transform(e.target.value) : e.target.value)} - {...fieldRest} - {...props} - /> - {/* todo: inline error message tooltip */} - -
+ + {({ id, 'aria-labelledby': ariaLabelledBy }) => ( + onChange(transform ? transform(e.target.value) : e.target.value)} + {...fieldRest} + {...props} + /> + )} + ) } diff --git a/app/forms/instance-create.tsx b/app/forms/instance-create.tsx index 4a24bccce4..153b0e6dff 100644 --- a/app/forms/instance-create.tsx +++ b/app/forms/instance-create.tsx @@ -46,13 +46,14 @@ import { DisksTableField, type DiskTableItem, } from '~/components/form/fields/DisksTableField' +import { FieldWrapper } from '~/components/form/fields/FieldWrapper' import { FileField } from '~/components/form/fields/FileField' import { BootDiskImageSelectField as ImageSelectField } from '~/components/form/fields/ImageSelectField' import { toIpPoolItem } from '~/components/form/fields/ip-pool-item' import { NameField } from '~/components/form/fields/NameField' import { NetworkInterfaceField } from '~/components/form/fields/NetworkInterfaceField' import { NumberField } from '~/components/form/fields/NumberField' -import { RadioFieldDyn } from '~/components/form/fields/RadioField' +import { RadioField } from '~/components/form/fields/RadioField' import { SshKeysField } from '~/components/form/fields/SshKeysField' import { TextField } from '~/components/form/fields/TextField' import { Form } from '~/components/form/Form' @@ -394,36 +395,21 @@ export default function CreateInstanceForm() { - + {renderLargeRadioCards('general')} - + - + {renderLargeRadioCards('highCPU')} - + - + {renderLargeRadioCards('highMemory')} - + @@ -749,21 +735,26 @@ const AdvancedAccordion = ({ Allocate and attach an ephemeral IP address {assignEphemeralIp && ( - pool.name === selectedPool)?.name}`} - items={siloPools.map(toIpPoolItem)} - disabled={!assignEphemeralIp || isSubmitting} - required - onChange={(value) => { - const newExternalIps = externalIps.field.value?.map((ip) => - ip.type === 'ephemeral' ? { ...ip, pool: value } : ip - ) - externalIps.field.onChange(newExternalIps) - }} - /> + + {({ id, 'aria-labelledby': ariaLabelledBy }) => ( + { + const newExternalIps = externalIps.field.value?.map((ip) => + ip.type === 'ephemeral' ? { ...ip, pool: value } : ip + ) + externalIps.field.onChange(newExternalIps) + }} + /> + )} + )} @@ -817,23 +808,28 @@ const AdvancedAccordion = ({
- ({ - value: i.name, - label: , - selectedLabel: `${i.name} (${i.ip})`, - }))} - label="Floating IP" - onChange={(name) => { - setSelectedFloatingIp( - availableFloatingIps.find((i) => i.name === name) - ) - }} - required - placeholder="Select a floating IP" - selected={selectedFloatingIp?.name || ''} - /> + + {({ id, 'aria-labelledby': ariaLabelledBy }) => ( + ({ + value: i.name, + label: , + selectedLabel: `${i.name} (${i.ip})`, + }))} + onChange={(name) => { + setSelectedFloatingIp( + availableFloatingIps.find((i) => i.name === name) + ) + }} + placeholder="Select a floating IP" + selected={selectedFloatingIp?.name || ''} + hasError={false} + aria-labelledby={ariaLabelledBy} + /> + )} +
diff --git a/app/ui/lib/Combobox.tsx b/app/ui/lib/Combobox.tsx index 5281e33821..db60898cae 100644 --- a/app/ui/lib/Combobox.tsx +++ b/app/ui/lib/Combobox.tsx @@ -15,11 +15,10 @@ import { } from '@headlessui/react' import cn from 'classnames' import { matchSorter } from 'match-sorter' -import { useEffect, useId, useState, type ReactNode, type Ref } from 'react' +import { useEffect, useState, type ReactNode, type Ref } from 'react' import { SelectArrows6Icon } from '@oxide/design-system/icons/react' -import { FieldLabel, InputHint } from './FieldLabel' import { usePopoverZIndex } from './SideModal' export type ComboboxItem = { value: string; label: ReactNode; selectedLabel: string } @@ -42,14 +41,10 @@ export const getSelectedLabelFromValue = ( /** Simple non-generic props shared with ComboboxField */ export type ComboboxBaseProps = { - description?: React.ReactNode disabled?: boolean isLoading?: boolean items: Array - label: string placeholder?: string - required?: boolean - hideOptionalTag?: boolean /** * Pass in `allowArbitraryValues` as `true` when the user should be able to * type in new values that aren't in the list [default is `false`] @@ -70,6 +65,7 @@ export type ComboboxBaseProps = { } type ComboboxProps = { + id: string selectedItemValue: string selectedItemLabel: string hasError?: boolean @@ -77,16 +73,16 @@ type ComboboxProps = { onChange: (value: string) => void /** Necessary if you want RHF to be able to focus it on error */ inputRef?: Ref + /** Accessibility labels */ + 'aria-labelledby'?: string } & ComboboxBaseProps export const Combobox = ({ - description, + id, items = [], - label, selectedItemValue, selectedItemLabel, placeholder, - required, hasError, disabled, isLoading, @@ -94,9 +90,9 @@ export const Combobox = ({ onEnter, onInputChange, allowArbitraryValues = false, - hideOptionalTag, inputRef, transform, + 'aria-labelledby': ariaLabelledBy, ...props }: ComboboxProps) => { const [query, setQuery] = useState(selectedItemValue || '') @@ -143,7 +139,6 @@ export const Combobox = ({ }) } const zIndex = usePopoverZIndex() - const id = useId() return ( {({ open }) => (
- {label && ( - // TODO: FieldLabel needs a real ID -
- - {label} - - {description && {description}} -
- )}
{items.length > 0 && ( { disabled?: boolean hasError?: boolean name?: string - label?: React.ReactNode - description?: React.ReactNode - required?: boolean + id?: string isLoading?: boolean /** Necessary if you want RHF to be able to focus it on error */ buttonRef?: Ref - hideOptionalTag?: boolean hideSelected?: boolean + /** Accessibility labels */ + 'aria-labelledby'?: string } export const Listbox = ({ @@ -54,21 +52,20 @@ export const Listbox = ({ className, onChange, hasError = false, - label, - description, - required, + id: idProp, disabled, isLoading = false, buttonRef, - hideOptionalTag, hideSelected = false, + 'aria-labelledby': ariaLabelledBy, ...props }: ListboxProps) => { const selectedItem = selected && items.find((i) => i.value === selected) const noItems = !isLoading && items.length === 0 const isDisabled = disabled || noItems const zIndex = usePopoverZIndex() - const id = useId() + const generatedId = useId() + const id = idProp || generatedId return (
@@ -82,18 +79,6 @@ export const Listbox = ({ > {({ open }) => (
- {label && ( -
- - {label} - - {description && {description}} -
- )} ({ hideSelected ? 'w-auto' : 'w-full' )} ref={buttonRef} + aria-labelledby={ariaLabelledBy} {...props} > {!hideSelected && ( From aba2b2d560147d9ad4c4be4ef1ec6c491b2d6add Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Thu, 14 Aug 2025 15:07:57 -0400 Subject: [PATCH 4/4] Set instance create form's Hardware radio buttons to render inline --- app/forms/instance-create.tsx | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/app/forms/instance-create.tsx b/app/forms/instance-create.tsx index 153b0e6dff..ee5086017a 100644 --- a/app/forms/instance-create.tsx +++ b/app/forms/instance-create.tsx @@ -395,19 +395,37 @@ export default function CreateInstanceForm() { - + {renderLargeRadioCards('general')} - + {renderLargeRadioCards('highCPU')} - + {renderLargeRadioCards('highMemory')}