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

Use NumberField instead of TextField for inputs of type number #1812

Closed
wants to merge 12 commits into from
2 changes: 1 addition & 1 deletion app/components/form/fields/DescriptionField.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ const MAX_LEN = 512
export function DescriptionField<
TFieldValues extends FieldValues,
TName extends FieldPath<TFieldValues>,
>(props: Omit<TextFieldProps<TFieldValues, TName>, 'validate'>) {
>(props: Omit<TextFieldProps<string, TFieldValues, TName>, 'validate'>) {
return <TextField as="textarea" validate={validateDescription} {...props} />
}

Expand Down
3 changes: 1 addition & 2 deletions app/components/form/fields/DiskSizeField.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import type { TextFieldProps } from './TextField'
interface DiskSizeProps<
TFieldValues extends FieldValues,
TName extends FieldPath<TFieldValues>,
> extends TextFieldProps<TFieldValues, TName> {
> extends TextFieldProps<number, TFieldValues, TName> {
minSize?: number
}

Expand All @@ -26,7 +26,6 @@ export function DiskSizeField<
return (
<NumberField
units="GiB"
type="number"
required={required}
name={name}
min={minSize}
Expand Down
2 changes: 1 addition & 1 deletion app/components/form/fields/NameField.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export function NameField<
name,
label = capitalize(name),
...textFieldProps
}: Omit<TextFieldProps<TFieldValues, TName>, 'validate'> & { label?: string }) {
}: Omit<TextFieldProps<string, TFieldValues, TName>, 'validate'> & { label?: string }) {
return (
<TextField
validate={(name) => validateName(name, label, required)}
Expand Down
12 changes: 9 additions & 3 deletions app/components/form/fields/NumberField.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export function NumberField<
helpText,
required,
...props
}: Omit<TextFieldProps<TFieldValues, TName>, 'id'>) {
}: Omit<TextFieldProps<number, TFieldValues, TName>, 'id' | 'type'>) {
// id is omitted from props because we generate it here
const id = useId()
return (
Expand Down Expand Up @@ -68,7 +68,10 @@ export const NumberFieldInner = <
description,
required,
id: idProp,
}: TextFieldProps<TFieldValues, TName>) => {
transform,
min,
max,
}: TextFieldProps<number, TFieldValues, TName>) => {
const generatedId = useId()
const id = idProp || generatedId

Expand All @@ -77,7 +80,7 @@ export const NumberFieldInner = <
name={name}
control={control}
rules={{ required, validate }}
render={({ field: { value, ...fieldRest }, fieldState: { error } }) => {
render={({ field: { value, onChange, ...fieldRest }, fieldState: { error } }) => {
return (
<>
<UINumberField
Expand All @@ -88,6 +91,9 @@ export const NumberFieldInner = <
})}
aria-describedby={description ? `${id}-label-tip` : undefined}
defaultValue={value}
onChange={(v) => onChange(transform ? transform(v) : v)}
minValue={typeof min !== 'undefined' ? Number(min) : undefined}
maxValue={typeof max !== 'undefined' ? Number(max) : undefined}
{...fieldRest}
/>
<ErrorMessage error={error} label={label} />
Expand Down
56 changes: 18 additions & 38 deletions app/components/form/fields/TextField.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* Copyright Oxide Computer Company
*/
import cn from 'classnames'
import { useId } from 'react'
import { useId, type HTMLInputTypeAttribute } from 'react'
import {
Controller,
type Control,
Expand All @@ -28,12 +28,13 @@ import { capitalize } from '@oxide/util'
import { ErrorMessage } from './ErrorMessage'

export interface TextFieldProps<
Type,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't actually like this change, it's a bit of a contagion. It's used to aid in typing the transform but I suspect there might be a better way to do this.

TFieldValues extends FieldValues,
TName extends FieldPath<TFieldValues>,
> extends UITextFieldProps {
> extends Omit<UITextFieldProps, 'type'> {
name: TName
/** HTML type attribute, defaults to text */
type?: string
type?: Omit<HTMLInputTypeAttribute, 'number'>
/** Will default to name if not provided */
label?: string
/**
Expand All @@ -56,6 +57,11 @@ export interface TextFieldProps<
units?: string
validate?: Validate<FieldPathValue<TFieldValues, TName>, TFieldValues>
control: Control<TFieldValues>
/**
* This function can be provided to alter the value of the input
* as the input is changed
*/
transform?: (value: Type) => Type | undefined
}

export function TextField<
Expand All @@ -69,7 +75,7 @@ export function TextField<
helpText,
required,
...props
}: Omit<TextFieldProps<TFieldValues, TName>, 'id'> & UITextAreaProps) {
}: Omit<TextFieldProps<string, TFieldValues, TName>, 'id'> & UITextAreaProps) {
// id is omitted from props because we generate it here
const id = useId()
return (
Expand All @@ -90,17 +96,9 @@ export function TextField<
)
}

function numberToInputValue(value: number) {
// could add `|| value === 0`, but that means when the value is 0, we always
// show an empty string, which is weird, and doubly bad because then the
// browser apparently fails to validate it against minimum (if one is
// provided). I found it let me submit instance create with 0 CPUs.
return isNaN(value) ? '' : value.toString()
}

/**
* Primarily exists for `TextField`, but we occasionally also need a plain field
* without a label on it. Note special handling of `type="number"`.
* 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
Expand All @@ -119,49 +117,31 @@ export const TextFieldInner = <
description,
required,
id: idProp,
transform,
...props
}: TextFieldProps<TFieldValues, TName> & UITextAreaProps) => {
}: TextFieldProps<string, TFieldValues, TName> & UITextAreaProps) => {
const generatedId = useId()
const id = idProp || generatedId
return (
<Controller
name={name}
control={control}
rules={{ required, validate }}
render={({ field: { onChange, value, ...fieldRest }, fieldState: { error } }) => {
render={({ field: { onChange, ...fieldRest }, fieldState: { error } }) => {
return (
<>
<UITextField
id={id}
title={label}
type={type}
type={type as string}
error={!!error}
aria-labelledby={cn(`${id}-label`, {
[`${id}-help-text`]: !!description,
})}
aria-describedby={description ? `${id}-label-tip` : undefined}
// note special handling for number fields, which produce a number
// for the calling code despite the actual input value necessarily
// being a string.
onChange={(e) => {
if (type === 'number') {
if (e.target.value.trim() === '') {
onChange(0)
} else if (!isNaN(e.target.valueAsNumber)) {
onChange(e.target.valueAsNumber)
}
// otherwise ignore the input. this means, for example, typing
// letters does nothing. If we instead said take anything
// that's NaN and fall back to 0, typing a letter would reset
// the field to 0, which is silly. Browsers are supposed to
// ignore non-numeric input for you anyway, but Firefox does
// not.
return
}

onChange(e.target.value)
}}
value={type === 'number' ? numberToInputValue(value) : value}
onChange={(e) =>
onChange(transform ? transform(e.target.value) : e.target.value)
}
{...fieldRest}
{...props}
/>
Expand Down
4 changes: 2 additions & 2 deletions app/forms/firewall-rules-create.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
SideModalForm,
TextField,
} from 'app/components/form'
import { NumberField } from 'app/components/form/fields/NumberField'
import { useForm, useVpcSelector } from 'app/hooks'

export type FirewallRuleValues = {
Expand Down Expand Up @@ -151,8 +152,7 @@ export const CommonFields = ({ error, control }: CommonFieldsProps) => {

<FormDivider />

<TextField
type="number"
<NumberField
name="priority"
helpText="Must be 0&ndash;65535"
required
Expand Down
7 changes: 3 additions & 4 deletions app/forms/instance-create.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ import {
TextField,
type DiskTableItem,
} from 'app/components/form'
import { NumberField } from 'app/components/form/fields/NumberField'
import { getProjectSelector, useForm, useProjectSelector, useToast } from 'app/hooks'
import { pb } from 'app/util/path-builder'

Expand Down Expand Up @@ -246,8 +247,7 @@ export function CreateInstanceForm() {
</Tabs.Content>

<Tabs.Content value="custom">
<TextField
type="number"
<NumberField
required
label="CPUs"
name="ncpus"
Expand All @@ -263,9 +263,8 @@ export function CreateInstanceForm() {
}
}}
/>
<TextField
<NumberField
units="GiB"
type="number"
required
label="Memory"
name="memory"
Expand Down
9 changes: 7 additions & 2 deletions app/forms/network-interface-create.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { useForm, useProjectSelector } from 'app/hooks'
const defaultValues: InstanceNetworkInterfaceCreate = {
name: '',
description: '',
ip: '',
ip: undefined,
subnetName: '',
vpcName: '',
}
Expand Down Expand Up @@ -80,7 +80,12 @@ export default function CreateNetworkInterfaceForm({
required
control={form.control}
/>
<TextField name="ip" label="IP Address" control={form.control} />
<TextField
name="ip"
label="IP Address"
control={form.control}
transform={(ip) => (ip.trim() === '' ? undefined : ip)}
/>
</SideModalForm>
)
}
2 changes: 1 addition & 1 deletion app/test/e2e/firewall-rules.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ test('can update firewall rule', async ({ page }) => {
await expect(page.locator('input[name=name]')).toHaveValue('allow-icmp')

// priority is populated
await expect(page.locator('input[name=priority]')).toHaveValue('65534')
await expect(page.locator('input[name=priority]')).toHaveValue('65,534')

// protocol is populated
await expect(page.locator('label >> text=ICMP')).toBeChecked()
Expand Down
Loading
Loading