Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion app/components/form/fields/DiskSizeField.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ export function DiskSizeField<
return (
<NumberField
units="GiB"
type="number"
required={required}
name={name}
min={minSize}
Expand Down
8 changes: 6 additions & 2 deletions app/components/form/fields/NumberField.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,9 @@ export function NumberField<
* 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 `TextField`, where it is always
* 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 `TextFieldInner` is used
* 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 = <
Expand All @@ -69,6 +69,8 @@ export const NumberFieldInner = <
required,
id: idProp,
disabled,
max,
min = 0,
}: TextFieldProps<TFieldValues, TName>) => {
const generatedId = useId()
const id = idProp || generatedId
Expand All @@ -89,6 +91,8 @@ export const NumberFieldInner = <
})}
aria-describedby={tooltipText ? `${id}-label-tip` : undefined}
isDisabled={disabled}
maxValue={max ? Number(max) : undefined}
minValue={min !== undefined ? Number(min) : undefined}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the min !== undefined construct is necessary because if min is 0, min ? __ : __ evaluates to false and minValue is set as undefined

{...field}
formatOptions={{
useGrouping: false,
Expand Down
39 changes: 4 additions & 35 deletions app/components/form/fields/TextField.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export interface TextFieldProps<
> extends UITextFieldProps {
name: TName
/** HTML type attribute, defaults to text */
type?: string
type?: 'text' | 'password'
/** Will default to name if not provided */
label?: string
/**
Expand Down Expand Up @@ -92,17 +92,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 Down Expand Up @@ -131,7 +123,7 @@ export const TextFieldInner = <
name={name}
control={control}
rules={{ required, validate }}
render={({ field: { onChange, value, ...fieldRest }, fieldState: { error } }) => {
render={({ field: { onChange, ...fieldRest }, fieldState: { error } }) => {
return (
<>
<UITextField
Expand All @@ -143,32 +135,9 @@ export const TextFieldInner = <
[`${id}-help-text`]: !!tooltipText,
})}
aria-describedby={tooltipText ? `${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 (transform) {
onChange(transform(e.target.value))
return
}
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)
onChange(transform ? transform(e.target.value) : e.target.value)
}}
value={type === 'number' ? numberToInputValue(value) : value}
Comment on lines 137 to -171
Copy link
Contributor

Choose a reason for hiding this comment

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

Ugh, I love to see it. Thank you @charliepark, this soothes my weary soul.

{...fieldRest}
{...props}
/>
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 {
ImageSelectField,
NameField,
NetworkInterfaceField,
NumberField,
RadioFieldDyn,
TextField,
type DiskTableItem,
Expand Down Expand Up @@ -284,8 +285,7 @@ export function CreateInstanceForm() {
</Tabs.Content>

<Tabs.Content value="custom">
<TextField
type="number"
<NumberField
required
label="CPUs"
name="ncpus"
Expand All @@ -302,9 +302,8 @@ export function CreateInstanceForm() {
}}
disabled={isSubmitting}
/>
<TextField
<NumberField
units="GiB"
type="number"
required
label="Memory"
name="memory"
Expand Down
3 changes: 0 additions & 3 deletions app/forms/silo-create.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,6 @@ export function CreateSiloSideModalForm() {
label="CPU quota"
name="quotas.cpus"
required
type="number"
units="nCPUs"
validate={validateQuota}
/>
Expand All @@ -117,7 +116,6 @@ export function CreateSiloSideModalForm() {
label="Memory quota"
name="quotas.memory"
required
type="number"
units="GiB"
validate={validateQuota}
/>
Expand All @@ -126,7 +124,6 @@ export function CreateSiloSideModalForm() {
label="Storage quota"
name="quotas.storage"
required
type="number"
units="GiB"
validate={validateQuota}
/>
Expand Down
4 changes: 2 additions & 2 deletions app/test/e2e/instance-create.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,8 @@ test('can create an instance with custom hardware', async ({ page }) => {

// Fill in custom specs
await page.getByRole('tab', { name: 'Custom' }).click()
await page.fill('input[name=ncpus]', '29')
await page.fill('input[name=memory]', '53')
await page.getByRole('textbox', { name: 'CPUs' }).fill('29')
await page.getByRole('textbox', { name: 'Memory (GiB)' }).fill('53')

await page.getByRole('textbox', { name: 'Disk name' }).fill('my-boot-disk')
const diskSizeInput = page.getByRole('textbox', { name: 'Disk size (GiB)' })
Expand Down