From a7376f768941cb55fd1c84e0403aaf2ea7800740 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Tue, 30 Jan 2024 20:03:16 -0800 Subject: [PATCH 1/6] Use NumberFields in more places; enhance NumberField component --- app/components/form/fields/DiskSizeField.tsx | 1 - app/components/form/fields/NumberField.tsx | 4 ++++ app/forms/instance-create.tsx | 7 +++---- app/forms/silo-create.tsx | 6 +++--- 4 files changed, 10 insertions(+), 8 deletions(-) diff --git a/app/components/form/fields/DiskSizeField.tsx b/app/components/form/fields/DiskSizeField.tsx index 7d97e3ca54..cf0a0a021a 100644 --- a/app/components/form/fields/DiskSizeField.tsx +++ b/app/components/form/fields/DiskSizeField.tsx @@ -38,7 +38,6 @@ export function DiskSizeField< return ( ) => { const generatedId = useId() const id = idProp || generatedId @@ -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} {...field} formatOptions={{ useGrouping: false, diff --git a/app/forms/instance-create.tsx b/app/forms/instance-create.tsx index 5ab813fef9..cc3ec9fc18 100644 --- a/app/forms/instance-create.tsx +++ b/app/forms/instance-create.tsx @@ -50,6 +50,7 @@ import { ImageSelectField, NameField, NetworkInterfaceField, + NumberField, RadioFieldDyn, TextField, type DiskTableItem, @@ -284,8 +285,7 @@ export function CreateInstanceForm() { - - From e3dba4ea02d6bce4a89d6f48a9fe9a29bdc47fb6 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Wed, 31 Jan 2024 09:02:36 -0800 Subject: [PATCH 2/6] Update tests --- app/test/e2e/instance-create.e2e.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/test/e2e/instance-create.e2e.ts b/app/test/e2e/instance-create.e2e.ts index 54ac350d44..bb023a95c1 100644 --- a/app/test/e2e/instance-create.e2e.ts +++ b/app/test/e2e/instance-create.e2e.ts @@ -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' }).fill('53') await page.getByRole('textbox', { name: 'Disk name' }).fill('my-boot-disk') const diskSizeInput = page.getByRole('textbox', { name: 'Disk size (GiB)' }) From cba5e66f3e4655bb421f8b132b73ade3b3f30f75 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Wed, 31 Jan 2024 10:18:21 -0800 Subject: [PATCH 3/6] Remove 'type' option from TextField; refactor onChange logic --- app/components/form/fields/NumberField.tsx | 4 +- app/components/form/fields/TextField.tsx | 43 ++-------------------- 2 files changed, 6 insertions(+), 41 deletions(-) diff --git a/app/components/form/fields/NumberField.tsx b/app/components/form/fields/NumberField.tsx index 50d015880a..99a862a51a 100644 --- a/app/components/form/fields/NumberField.tsx +++ b/app/components/form/fields/NumberField.tsx @@ -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 = < diff --git a/app/components/form/fields/TextField.tsx b/app/components/form/fields/TextField.tsx index e341be08aa..456f2aac1c 100644 --- a/app/components/form/fields/TextField.tsx +++ b/app/components/form/fields/TextField.tsx @@ -30,10 +30,8 @@ import { ErrorMessage } from './ErrorMessage' export interface TextFieldProps< TFieldValues extends FieldValues, TName extends FieldPath, -> extends UITextFieldProps { +> extends Omit { name: TName - /** HTML type attribute, defaults to text */ - type?: string /** Will default to name if not provided */ label?: string /** @@ -92,17 +90,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 @@ -114,7 +104,6 @@ export const TextFieldInner = < TName extends FieldPath, >({ name, - type = 'text', label = capitalize(name), validate, control, @@ -131,44 +120,20 @@ export const TextFieldInner = < name={name} control={control} rules={{ required, validate }} - render={({ field: { onChange, value, ...fieldRest }, fieldState: { error } }) => { + render={({ field: { onChange, ...fieldRest }, fieldState: { error } }) => { return ( <> { - 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} {...fieldRest} {...props} /> From ecbee117dcb0e0cbcefad5d111c8a963b3805259 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Wed, 31 Jan 2024 10:24:32 -0800 Subject: [PATCH 4/6] Apply default minimum of 0 on NumberFields --- app/components/form/fields/NumberField.tsx | 2 +- app/forms/silo-create.tsx | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/app/components/form/fields/NumberField.tsx b/app/components/form/fields/NumberField.tsx index 99a862a51a..ddc1cd8142 100644 --- a/app/components/form/fields/NumberField.tsx +++ b/app/components/form/fields/NumberField.tsx @@ -70,7 +70,7 @@ export const NumberFieldInner = < id: idProp, disabled, max, - min, + min = 0, }: TextFieldProps) => { const generatedId = useId() const id = idProp || generatedId diff --git a/app/forms/silo-create.tsx b/app/forms/silo-create.tsx index 39bd08a118..7cde774aba 100644 --- a/app/forms/silo-create.tsx +++ b/app/forms/silo-create.tsx @@ -109,7 +109,6 @@ export function CreateSiloSideModalForm() { name="quotas.cpus" required units="nCPUs" - min={0} validate={validateQuota} /> From f624c15ecbb013880de4ac74201d40f0bd5bd093 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Wed, 31 Jan 2024 11:27:50 -0800 Subject: [PATCH 5/6] Allow for the possibility of password types in TextFields --- app/components/form/fields/TextField.tsx | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/components/form/fields/TextField.tsx b/app/components/form/fields/TextField.tsx index 456f2aac1c..4d561c8585 100644 --- a/app/components/form/fields/TextField.tsx +++ b/app/components/form/fields/TextField.tsx @@ -30,8 +30,10 @@ import { ErrorMessage } from './ErrorMessage' export interface TextFieldProps< TFieldValues extends FieldValues, TName extends FieldPath, -> extends Omit { +> extends UITextFieldProps { name: TName + /** HTML type attribute, defaults to text */ + type?: 'text' | 'password' /** Will default to name if not provided */ label?: string /** @@ -104,6 +106,7 @@ export const TextFieldInner = < TName extends FieldPath, >({ name, + type = 'text', label = capitalize(name), validate, control, @@ -126,6 +129,7 @@ export const TextFieldInner = < Date: Wed, 31 Jan 2024 11:40:06 -0800 Subject: [PATCH 6/6] More explicit test querying --- app/test/e2e/instance-create.e2e.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/test/e2e/instance-create.e2e.ts b/app/test/e2e/instance-create.e2e.ts index bb023a95c1..49f81c33a6 100644 --- a/app/test/e2e/instance-create.e2e.ts +++ b/app/test/e2e/instance-create.e2e.ts @@ -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.getByRole('textbox', { name: 'cpus' }).fill('29') - await page.getByRole('textbox', { name: 'memory' }).fill('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)' })