Use NumberFields in more places; enhance NumberField component (#1926)

* Use NumberFields in more places; enhance NumberField component
* Apply default minimum of 0 on NumberFields
* Refactor onChange logic in TextFields, permit 'text' and 'password' types

Author: charliepark

app/components/form/fields/DiskSizeField.tsx

@@ -38,7 +38,6 @@ export function DiskSizeField<
   return (
     <NumberField
       units="GiB"
-      type="number"
       required={required}
       name={name}
       min={minSize}

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 = <
@@ -69,6 +69,8 @@ export const NumberFieldInner = <
   required,
   id: idProp,
   disabled,
+  max,
+  min = 0,
 }: TextFieldProps<TFieldValues, TName>) => {
   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,

app/components/form/fields/TextField.tsx

@@ -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
   /**
@@ -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
@@ -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
@@ -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}
               {...fieldRest}
               {...props}
             />

app/forms/instance-create.tsx

@@ -46,6 +46,7 @@ import {
   ImageSelectField,
   NameField,
   NetworkInterfaceField,
+  NumberField,
   RadioFieldDyn,
   SshKeysField,
   TextField,
@@ -290,8 +291,7 @@ export function CreateInstanceForm() {
         </Tabs.Content>
 
         <Tabs.Content value="custom">
-          <TextField
-            type="number"
+          <NumberField
             required
             label="CPUs"
             name="ncpus"
@@ -308,9 +308,8 @@ export function CreateInstanceForm() {
             }}
             disabled={isSubmitting}
           />
-          <TextField
+          <NumberField
             units="GiB"
-            type="number"
             required
             label="Memory"
             name="memory"

app/forms/silo-create.tsx

@@ -108,7 +108,6 @@ export function CreateSiloSideModalForm() {
         label="CPU quota"
         name="quotas.cpus"
         required
-        type="number"
         units="nCPUs"
         validate={validateQuota}
       />
@@ -117,7 +116,6 @@ export function CreateSiloSideModalForm() {
         label="Memory quota"
         name="quotas.memory"
         required
-        type="number"
         units="GiB"
         validate={validateQuota}
       />
@@ -126,7 +124,6 @@ export function CreateSiloSideModalForm() {
         label="Storage quota"
         name="quotas.storage"
         required
-        type="number"
         units="GiB"
         validate={validateQuota}
       />

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 (GiB)' }).fill('53')
 
   await page.getByRole('textbox', { name: 'Disk name' }).fill('my-boot-disk')
   const diskSizeInput = page.getByRole('textbox', { name: 'Disk size (GiB)' })