From 93a52937c5372aeef974a9f849c1c9778302cd96 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Fri, 8 Nov 2024 01:40:22 -0800 Subject: [PATCH 01/12] Handle enter keypress in comboboxes --- app/components/form/fields/ComboboxField.tsx | 3 + app/forms/firewall-rules-common.tsx | 65 ++++++++++---------- app/ui/lib/Combobox.tsx | 3 + app/ui/lib/MiniTable.tsx | 4 +- 4 files changed, 40 insertions(+), 35 deletions(-) diff --git a/app/components/form/fields/ComboboxField.tsx b/app/components/form/fields/ComboboxField.tsx index a49561b7dd..6d782de640 100644 --- a/app/components/form/fields/ComboboxField.tsx +++ b/app/components/form/fields/ComboboxField.tsx @@ -32,6 +32,7 @@ export type ComboboxFieldProps< name: TName control: Control onChange?: (value: string | null | undefined) => void + onKeyDown?: (event: React.KeyboardEvent) => void validate?: Validate, TFieldValues> } & ComboboxBaseProps @@ -45,6 +46,7 @@ export function ComboboxField< label = capitalize(name), required, onChange, + onKeyDown, allowArbitraryValues, placeholder, // Intent is to not show both a placeholder and a description, while still having good defaults; prefer a description to a placeholder @@ -87,6 +89,7 @@ export function ComboboxField< onChange?.(value) setSelectedItemLabel(getSelectedLabelFromValue(items, value)) }} + onKeyDown={onKeyDown} allowArbitraryValues={allowArbitraryValues} inputRef={field.ref} {...props} diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index da348ee214..89b185263c 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -6,12 +6,13 @@ * Copyright Oxide Computer Company */ -import { useEffect } from 'react' +import { useEffect, useRef } from 'react' import { useController, useForm, type Control, type ControllerRenderProps, + type UseFormReturn, } from 'react-hook-form' import { @@ -91,23 +92,28 @@ const getFilterValueProps = (targetOrHostType: TargetAndHostFilterType) => { const DynamicTypeAndValueFields = ({ sectionType, - control, valueType, + form, items, disabled, - onInputChange, - onTypeChange, + disableClear, onSubmitTextField, }: { sectionType: 'target' | 'host' - control: Control valueType: TargetAndHostFilterType + form: UseFormReturn items: Array disabled?: boolean - onInputChange?: (value: string) => void - onTypeChange: () => void - onSubmitTextField: (e: React.KeyboardEvent) => void + disableClear: boolean + onSubmitTextField: (e?: React.KeyboardEvent) => void }) => { + const control = form.control + // HACK: reset the whole subform, keeping type (because we just set + // it). most importantly, this resets isSubmitted so the form can go + // back to validating on submit instead of change + const onTypeChange = () => form.reset({ type: form.getValues('type'), value: '' }) + const onInputChange = (value: string) => form.setValue('value', value) + const addButtonRef = useRef(null) return ( <> ) => { + if (e.key === KEYS.enter) { + // e.preventDefault() // prevent full form submission + addButtonRef.current?.focus() // focus on the mini form's add button + } + }} onInputChange={onInputChange} items={items} allowArbitraryValues @@ -162,6 +174,13 @@ const DynamicTypeAndValueFields = ({ } /> )} + form.reset()} + onSubmit={onSubmitTextField} + /> ) } @@ -438,23 +457,11 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = - targetForm.reset({ type: targetForm.getValues('type'), value: '' }) - } - onInputChange={(value) => targetForm.setValue('value', value)} - onSubmitTextField={submitTarget} - /> - targetForm.reset()} - onSubmit={submitTarget} + onSubmitTextField={submitTarget} /> {!!targets.value.length && } @@ -554,21 +561,11 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = /> hostForm.reset({ type: hostForm.getValues('type'), value: '' })} - onInputChange={(value) => hostForm.setValue('value', value)} - onSubmitTextField={submitHost} - /> - hostForm.reset()} - onSubmit={submitHost} + onSubmitTextField={submitHost} /> {!!hosts.value.length && } diff --git a/app/ui/lib/Combobox.tsx b/app/ui/lib/Combobox.tsx index f9cbc7cf45..226b243c00 100644 --- a/app/ui/lib/Combobox.tsx +++ b/app/ui/lib/Combobox.tsx @@ -69,6 +69,7 @@ type ComboboxProps = { selectedItemLabel: string hasError?: boolean onChange: (value: string) => void + onKeyDown?: (event: React.KeyboardEvent) => void /** Necessary if you want RHF to be able to focus it on error */ inputRef?: Ref } & ComboboxBaseProps @@ -85,6 +86,7 @@ export const Combobox = ({ disabled, isLoading, onChange, + onKeyDown, onInputChange, allowArbitraryValues = false, hideOptionalTag, @@ -196,6 +198,7 @@ export const Combobox = ({ // if the parent component wants to know about input changes, call the callback onInputChange?.(event.target.value) }} + onKeyDown={onKeyDown} placeholder={placeholder} disabled={disabled || isLoading} className={cn( diff --git a/app/ui/lib/MiniTable.tsx b/app/ui/lib/MiniTable.tsx index b09383cf22..69c054a6eb 100644 --- a/app/ui/lib/MiniTable.tsx +++ b/app/ui/lib/MiniTable.tsx @@ -48,6 +48,7 @@ export const RemoveCell = ({ onClick, label }: { onClick: () => void; label: str type ClearAndAddButtonsProps = { addButtonCopy: string + addButtonRef?: React.RefObject disableClear: boolean onClear: () => void onSubmit: () => void @@ -59,6 +60,7 @@ type ClearAndAddButtonsProps = { */ export const ClearAndAddButtons = ({ addButtonCopy, + addButtonRef, disableClear, onClear, onSubmit, @@ -67,7 +69,7 @@ export const ClearAndAddButtons = ({ - From e0ee6fe81d843a0e748c9d4e015246fad34f5da6 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Fri, 8 Nov 2024 02:21:13 -0800 Subject: [PATCH 02/12] refactor target and host filter forms --- app/forms/firewall-rules-common.tsx | 73 +++++++++++------------------ 1 file changed, 28 insertions(+), 45 deletions(-) diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 89b185263c..cac59c4f4d 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -35,7 +35,7 @@ import { RadioField } from '~/components/form/fields/RadioField' import { TextField, TextFieldInner } from '~/components/form/fields/TextField' import { useVpcSelector } from '~/hooks/use-params' import { Badge } from '~/ui/lib/Badge' -import { toComboboxItems, type ComboboxItem } from '~/ui/lib/Combobox' +import { toComboboxItems } from '~/ui/lib/Combobox' import { FormDivider } from '~/ui/lib/Divider' import { FieldLabel } from '~/ui/lib/FieldLabel' import { Message } from '~/ui/lib/Message' @@ -92,21 +92,39 @@ const getFilterValueProps = (targetOrHostType: TargetAndHostFilterType) => { const DynamicTypeAndValueFields = ({ sectionType, - valueType, form, - items, + field, disabled, - disableClear, onSubmitTextField, }: { sectionType: 'target' | 'host' - valueType: TargetAndHostFilterType form: UseFormReturn - items: Array + field: ControllerRenderProps disabled?: boolean - disableClear: boolean onSubmitTextField: (e?: React.KeyboardEvent) => void }) => { + const { project, vpc } = useVpcSelector() + // prefetchedApiQueries below are prefetched in firewall-rules-create and -edit + const { + data: { items: instances }, + } = usePrefetchedApiQuery('instanceList', { query: { project, limit: ALL_ISH } }) + const { + data: { items: vpcs }, + } = usePrefetchedApiQuery('vpcList', { query: { project, limit: ALL_ISH } }) + const { + data: { items: vpcSubnets }, + } = usePrefetchedApiQuery('vpcSubnetList', { query: { project, vpc } }) + + const valueType = form.watch('type') + const value = form.watch('value') + const sectionItems = { + vpc: availableItems(field.value, 'vpc', vpcs), + subnet: availableItems(field.value, 'subnet', vpcSubnets), + instance: availableItems(field.value, 'instance', instances), + ip: [], + ip_net: [], + } + const items = toComboboxItems(sectionItems[valueType]) const control = form.control // HACK: reset the whole subform, keeping type (because we just set // it). most importantly, this resets isSubmitted so the form can go @@ -177,7 +195,7 @@ const DynamicTypeAndValueFields = ({ form.reset()} onSubmit={onSubmitTextField} /> @@ -268,31 +286,9 @@ const targetAndHostDefaultValues: TargetAndHostFormValues = { } export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) => { - const { project, vpc } = useVpcSelector() - - // prefetchedApiQueries below are prefetched in firewall-rules-create and -edit - const { - data: { items: instances }, - } = usePrefetchedApiQuery('instanceList', { query: { project, limit: ALL_ISH } }) - const { - data: { items: vpcs }, - } = usePrefetchedApiQuery('vpcList', { query: { project, limit: ALL_ISH } }) - const { - data: { items: vpcSubnets }, - } = usePrefetchedApiQuery('vpcSubnetList', { query: { project, vpc } }) - // Targets const targetForm = useForm({ defaultValues: targetAndHostDefaultValues }) const targets = useController({ name: 'targets', control }).field - const [targetType, targetValue] = targetForm.watch(['type', 'value']) - // get the list of items that are not already in the list of targets - const targetItems = { - vpc: availableItems(targets.value, 'vpc', vpcs), - subnet: availableItems(targets.value, 'subnet', vpcSubnets), - instance: availableItems(targets.value, 'instance', instances), - ip: [], - ip_net: [], - } const submitTarget = targetForm.handleSubmit(({ type, value }) => { // TODO: do this with a normal validation // ignore click if empty or a duplicate @@ -330,15 +326,6 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = // Host Filters const hostForm = useForm({ defaultValues: targetAndHostDefaultValues }) const hosts = useController({ name: 'hosts', control }).field - const [hostType, hostValue] = hostForm.watch(['type', 'value']) - // get the list of items that are not already in the list of host filters - const hostFilterItems = { - vpc: availableItems(hosts.value, 'vpc', vpcs), - subnet: availableItems(hosts.value, 'subnet', vpcSubnets), - instance: availableItems(hosts.value, 'instance', instances), - ip: [], - ip_net: [], - } const submitHost = hostForm.handleSubmit(({ type, value }) => { // ignore click if empty or a duplicate // TODO: show error instead of ignoring click @@ -457,10 +444,8 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = {!!targets.value.length && } @@ -561,10 +546,8 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = /> {!!hosts.value.length && } From 9af9aac23fb505b285fdcb08c9f3b5be78e01f68 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Fri, 8 Nov 2024 09:04:34 -0800 Subject: [PATCH 03/12] Continued simplification of shared form UI --- app/forms/firewall-rules-common.tsx | 124 +++++++++++----------------- 1 file changed, 46 insertions(+), 78 deletions(-) diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index cac59c4f4d..e25e6d5869 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -12,7 +12,6 @@ import { useForm, type Control, type ControllerRenderProps, - type UseFormReturn, } from 'react-hook-form' import { @@ -92,16 +91,10 @@ const getFilterValueProps = (targetOrHostType: TargetAndHostFilterType) => { const DynamicTypeAndValueFields = ({ sectionType, - form, - field, - disabled, - onSubmitTextField, + control, }: { sectionType: 'target' | 'host' - form: UseFormReturn - field: ControllerRenderProps - disabled?: boolean - onSubmitTextField: (e?: React.KeyboardEvent) => void + control: Control }) => { const { project, vpc } = useVpcSelector() // prefetchedApiQueries below are prefetched in firewall-rules-create and -edit @@ -115,8 +108,34 @@ const DynamicTypeAndValueFields = ({ data: { items: vpcSubnets }, } = usePrefetchedApiQuery('vpcSubnetList', { query: { project, vpc } }) - const valueType = form.watch('type') - const value = form.watch('value') + const subform = useForm({ defaultValues: targetAndHostDefaultValues }) + const field = useController({ name: `${sectionType}s`, control }).field + + const submitSubform = subform.handleSubmit(({ type, value }) => { + // TODO: do this with a normal validation + // ignore click if empty or a duplicate + // TODO: show error instead of ignoring click + if (!type || !value) return + if (field.value.some((f) => f.value === value && f.type === type)) return + field.onChange([...field.value, { type, value }]) + subform.reset(targetAndHostDefaultValues) + }) + + // HACK: we need to reset the target form completely after a successful submit, + // including especially `isSubmitted`, because that governs whether we're in + // the validate regime (which doesn't validate until submit) or the reValidate + // regime (which validate on every keypress). The reset inside `handleSubmit` + // doesn't do that for us because `handleSubmit` set `isSubmitted: true` after + // running the callback. So we have to watch for a successful submit and call + // the reset again here. + // https://github.com/react-hook-form/react-hook-form/blob/9a497a70a/src/logic/createFormControl.ts#L1194-L1203 + const { isSubmitSuccessful: subformSubmitSuccessful } = subform.formState + useEffect(() => { + if (subformSubmitSuccessful) subform.reset(targetAndHostDefaultValues) + }, [subformSubmitSuccessful, subform]) + + const valueType = subform.watch('type') + const value = subform.watch('value') const sectionItems = { vpc: availableItems(field.value, 'vpc', vpcs), subnet: availableItems(field.value, 'subnet', vpcSubnets), @@ -125,19 +144,19 @@ const DynamicTypeAndValueFields = ({ ip_net: [], } const items = toComboboxItems(sectionItems[valueType]) - const control = form.control + const subformControl = subform.control // HACK: reset the whole subform, keeping type (because we just set // it). most importantly, this resets isSubmitted so the form can go // back to validating on submit instead of change - const onTypeChange = () => form.reset({ type: form.getValues('type'), value: '' }) - const onInputChange = (value: string) => form.setValue('value', value) + const onTypeChange = () => subform.reset({ type: subform.getValues('type'), value: '' }) + const onInputChange = (value: string) => subform.setValue('value', value) const addButtonRef = useRef(null) return ( <> ) => { if (e.key === KEYS.enter) { // e.preventDefault() // prevent full form submission @@ -178,11 +197,12 @@ const DynamicTypeAndValueFields = ({ { if (e.key === KEYS.enter) { e.preventDefault() // prevent full form submission - onSubmitTextField(e) + submitSubform(e) } }} validate={(value) => @@ -196,9 +216,12 @@ const DynamicTypeAndValueFields = ({ addButtonCopy={`Add ${sectionType === 'host' ? 'host filter' : 'target'}`} addButtonRef={addButtonRef} disableClear={!value} - onClear={() => form.reset()} - onSubmit={onSubmitTextField} + onClear={() => subform.reset()} + onSubmit={submitSubform} /> + {!!field.value.length && ( + + )} ) } @@ -286,31 +309,6 @@ const targetAndHostDefaultValues: TargetAndHostFormValues = { } export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) => { - // Targets - const targetForm = useForm({ defaultValues: targetAndHostDefaultValues }) - const targets = useController({ name: 'targets', control }).field - const submitTarget = targetForm.handleSubmit(({ type, value }) => { - // TODO: do this with a normal validation - // ignore click if empty or a duplicate - // TODO: show error instead of ignoring click - if (!type || !value) return - if (targets.value.some((t) => t.value === value && t.type === type)) return - targets.onChange([...targets.value, { type, value }]) - targetForm.reset(targetAndHostDefaultValues) - }) - // HACK: we need to reset the target form completely after a successful submit, - // including especially `isSubmitted`, because that governs whether we're in - // the validate regime (which doesn't validate until submit) or the reValidate - // regime (which validate on every keypress). The reset inside `handleSubmit` - // doesn't do that for us because `handleSubmit` set `isSubmitted: true` after - // running the callback. So we have to watch for a successful submit and call - // the reset again here. - // https://github.com/react-hook-form/react-hook-form/blob/9a497a70a/src/logic/createFormControl.ts#L1194-L1203 - const { isSubmitSuccessful: targetSubmitSuccessful } = targetForm.formState - useEffect(() => { - if (targetSubmitSuccessful) targetForm.reset(targetAndHostDefaultValues) - }, [targetSubmitSuccessful, targetForm]) - // Ports const portRangeForm = useForm({ defaultValues: { portRange: '' } }) const ports = useController({ name: 'ports', control }).field @@ -323,23 +321,6 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = portRangeForm.reset() }) - // Host Filters - const hostForm = useForm({ defaultValues: targetAndHostDefaultValues }) - const hosts = useController({ name: 'hosts', control }).field - const submitHost = hostForm.handleSubmit(({ type, value }) => { - // ignore click if empty or a duplicate - // TODO: show error instead of ignoring click - if (!type || !value) return - if (hosts.value.some((t) => t.value === value && t.type === type)) return - hosts.onChange([...hosts.value, { type, value }]) - hostForm.reset(targetAndHostDefaultValues) - }) - // HACK: see comment above about doing the same for target form - const { isSubmitSuccessful: hostSubmitSuccessful } = hostForm.formState - useEffect(() => { - if (hostSubmitSuccessful) hostForm.reset(targetAndHostDefaultValues) - }, [hostSubmitSuccessful, hostForm]) - return ( <> } /> - - - {!!targets.value.length && } + @@ -544,13 +518,7 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = } /> - - {!!hosts.value.length && } + {error && ( <> From 02392bbb32741c348efde5414abdc874d9180dc7 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Fri, 8 Nov 2024 09:37:31 -0800 Subject: [PATCH 04/12] the hits keep coming --- app/forms/firewall-rules-common.tsx | 121 +++++++++++++--------------- 1 file changed, 54 insertions(+), 67 deletions(-) diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index e25e6d5869..a66ff48eb5 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -6,13 +6,8 @@ * Copyright Oxide Computer Company */ -import { useEffect, useRef } from 'react' -import { - useController, - useForm, - type Control, - type ControllerRenderProps, -} from 'react-hook-form' +import { useEffect, useRef, type ReactNode } from 'react' +import { useController, useForm, type Control } from 'react-hook-form' import { usePrefetchedApiQuery, @@ -55,7 +50,7 @@ import { type FirewallRuleValues } from './firewall-rules-util' * a few sub-sections (Ports, Protocols, and Hosts). * * The Targets section and the Filters:Hosts section are very similar, so we've - * pulled common code to the DynamicTypeAndValueFields components. + * pulled common code to the TargetAndHostFilterSubform components. * We also then set up the Targets / Ports / Hosts variables at the top of the * CommonFields component. */ @@ -89,12 +84,14 @@ const getFilterValueProps = (targetOrHostType: TargetAndHostFilterType) => { } } -const DynamicTypeAndValueFields = ({ +const TargetAndHostFilterSubform = ({ sectionType, control, + messageContent, }: { sectionType: 'target' | 'host' control: Control + messageContent: ReactNode }) => { const { project, vpc } = useVpcSelector() // prefetchedApiQueries below are prefetched in firewall-rules-create and -edit @@ -134,8 +131,7 @@ const DynamicTypeAndValueFields = ({ if (subformSubmitSuccessful) subform.reset(targetAndHostDefaultValues) }, [subformSubmitSuccessful, subform]) - const valueType = subform.watch('type') - const value = subform.watch('value') + const [valueType, value] = subform.watch(['type', 'value']) const sectionItems = { vpc: availableItems(field.value, 'vpc', vpcs), subnet: availableItems(field.value, 'subnet', vpcSubnets), @@ -153,6 +149,11 @@ const DynamicTypeAndValueFields = ({ const addButtonRef = useRef(null) return ( <> + + {sectionType === 'target' ? 'Targets' : 'Host filters'} + + + {!!field.value.length && ( - + + + Type + Value + {/* For remove button */} + + + + {field.value.map(({ type, value }, index) => ( + + + {type} + + {value} + + field.onChange( + field.value.filter((i) => !(i.value === value && i.type === type)) + ) + } + label={`remove ${sectionType} ${value}`} + /> + + ))} + + )} ) } -type TypeAndValueTableProps = { - sectionType: 'target' | 'host' - items: ControllerRenderProps -} -const TypeAndValueTable = ({ sectionType, items }: TypeAndValueTableProps) => ( - - - Type - Value - {/* For remove button */} - - - - {items.value.map(({ type, value }, index) => ( - - - {type} - - {value} - - items.onChange( - items.value.filter((i) => !(i.value === value && i.type === type)) - ) - } - label={`remove ${sectionType} ${value}`} - /> - - ))} - - -) - /** Given an array of *committed* items (VPCs, Subnets, Instances) and a list of *all* items, * return the items that are available */ const availableItems = ( @@ -402,13 +395,10 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = - {/* Really this should be its own
, but you can't have a form inside a form, - so we just stick the submit handler in a button onClick */} - Targets - -

Targets determine the instances to which this rule applies. You can target @@ -422,7 +412,6 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = } /> - @@ -506,11 +495,10 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = - Host filters - - Host filters match the “other end” of traffic from the target’s perspective: for an inbound rule, they match the source of @@ -518,7 +506,6 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = } /> - {error && ( <> From b3aa75f35c932991726bdfdf430cf4b20547b681 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Fri, 8 Nov 2024 09:51:22 -0800 Subject: [PATCH 05/12] add tests re: hitting enter in subform --- app/forms/firewall-rules-common.tsx | 3 +-- test/e2e/firewall-rules.e2e.ts | 4 ++++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index a66ff48eb5..4605aa1806 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -178,8 +178,7 @@ const TargetAndHostFilterSubform = ({ control={subformControl} onKeyDown={(e: React.KeyboardEvent) => { if (e.key === KEYS.enter) { - // e.preventDefault() // prevent full form submission - addButtonRef.current?.focus() // focus on the mini form's add button + addButtonRef.current?.focus() // focus on the subform's `add _____` button } }} onInputChange={onInputChange} diff --git a/test/e2e/firewall-rules.e2e.ts b/test/e2e/firewall-rules.e2e.ts index 1cd693b813..d9f6c7b2b7 100644 --- a/test/e2e/firewall-rules.e2e.ts +++ b/test/e2e/firewall-rules.e2e.ts @@ -164,6 +164,8 @@ test('firewall rule form targets table', async ({ page }) => { await targetVpcNameField.fill('abc') await targetVpcNameField.press('Enter') + // expect that the addButton has focus + await expect(addButton).toBeFocused() await addButton.click() await expectRowVisible(targets, { Type: 'vpc', Value: 'abc' }) @@ -187,6 +189,8 @@ test('firewall rule form targets table', async ({ page }) => { await selectOption(page, 'Target type', 'VPC subnet') await subnetNameField.fill('abc') await page.getByText('abc').first().click() + // hit enter to move focus to the add button + await subnetNameField.press('Enter') await addButton.click() await expectRowVisible(targets, { Type: 'subnet', Value: 'abc' }) From 408bba70070f0ebb29999712b7662bb0e1e11ea0 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Mon, 11 Nov 2024 14:31:54 -0800 Subject: [PATCH 06/12] Update interactivity to better handle hitting enter in combobox --- app/forms/firewall-rules-common.tsx | 34 ++++++++++++++++++++++------- app/ui/lib/MiniTable.tsx | 4 +--- test/e2e/firewall-rules.e2e.ts | 11 +++++----- 3 files changed, 32 insertions(+), 17 deletions(-) diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 4605aa1806..4dfe2a649a 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -6,7 +6,7 @@ * Copyright Oxide Computer Company */ -import { useEffect, useRef, type ReactNode } from 'react' +import { useEffect, useState, type ReactNode } from 'react' import { useController, useForm, type Control } from 'react-hook-form' import { @@ -105,6 +105,8 @@ const TargetAndHostFilterSubform = ({ data: { items: vpcSubnets }, } = usePrefetchedApiQuery('vpcSubnetList', { query: { project, vpc } }) + const [readyToSubmit, setReadyToSubmit] = useState(false) + const subform = useForm({ defaultValues: targetAndHostDefaultValues }) const field = useController({ name: `${sectionType}s`, control }).field @@ -116,6 +118,7 @@ const TargetAndHostFilterSubform = ({ if (field.value.some((f) => f.value === value && f.type === type)) return field.onChange([...field.value, { type, value }]) subform.reset(targetAndHostDefaultValues) + setReadyToSubmit(false) }) // HACK: we need to reset the target form completely after a successful submit, @@ -143,10 +146,16 @@ const TargetAndHostFilterSubform = ({ const subformControl = subform.control // HACK: reset the whole subform, keeping type (because we just set // it). most importantly, this resets isSubmitted so the form can go - // back to validating on submit instead of change - const onTypeChange = () => subform.reset({ type: subform.getValues('type'), value: '' }) - const onInputChange = (value: string) => subform.setValue('value', value) - const addButtonRef = useRef(null) + // back to validating on submit instead of change. Also resets readyToSubmit. + const onTypeChange = () => { + subform.reset({ type: subform.getValues('type'), value: '' }) + setReadyToSubmit(false) + } + const onInputChange = (value: string) => { + subform.setValue('value', value) + setReadyToSubmit(false) + } + return ( <> @@ -177,10 +186,20 @@ const TargetAndHostFilterSubform = ({ description="Select an option or enter a custom value" control={subformControl} onKeyDown={(e: React.KeyboardEvent) => { - if (e.key === KEYS.enter) { - addButtonRef.current?.focus() // focus on the subform's `add _____` button + if (e.key === KEYS.down || e.key === KEYS.up) { + setReadyToSubmit(false) + } + if (e.key === KEYS.enter && value.length) { + // the first time they hit the enter key, set readyToSubmit to true, + // so that the second time they hit enter, it submits the subform + if (!readyToSubmit) { + setReadyToSubmit(true) + } else { + submitSubform(e) + } } }} + onChange={() => setReadyToSubmit(true)} onInputChange={onInputChange} items={items} allowArbitraryValues @@ -214,7 +233,6 @@ const TargetAndHostFilterSubform = ({ )} subform.reset()} onSubmit={submitSubform} diff --git a/app/ui/lib/MiniTable.tsx b/app/ui/lib/MiniTable.tsx index 69c054a6eb..b09383cf22 100644 --- a/app/ui/lib/MiniTable.tsx +++ b/app/ui/lib/MiniTable.tsx @@ -48,7 +48,6 @@ export const RemoveCell = ({ onClick, label }: { onClick: () => void; label: str type ClearAndAddButtonsProps = { addButtonCopy: string - addButtonRef?: React.RefObject disableClear: boolean onClear: () => void onSubmit: () => void @@ -60,7 +59,6 @@ type ClearAndAddButtonsProps = { */ export const ClearAndAddButtons = ({ addButtonCopy, - addButtonRef, disableClear, onClear, onSubmit, @@ -69,7 +67,7 @@ export const ClearAndAddButtons = ({ - diff --git a/test/e2e/firewall-rules.e2e.ts b/test/e2e/firewall-rules.e2e.ts index d9f6c7b2b7..7fd100fd34 100644 --- a/test/e2e/firewall-rules.e2e.ts +++ b/test/e2e/firewall-rules.e2e.ts @@ -163,10 +163,10 @@ test('firewall rule form targets table', async ({ page }) => { // add targets with overlapping names and types to test delete await targetVpcNameField.fill('abc') + // hit enter one time to choose the custom value + await targetVpcNameField.press('Enter') + // hit enter a second time to submit the subform await targetVpcNameField.press('Enter') - // expect that the addButton has focus - await expect(addButton).toBeFocused() - await addButton.click() await expectRowVisible(targets, { Type: 'vpc', Value: 'abc' }) // enter a VPC called 'mock-subnet', even if that doesn't make sense here, to test dropdown later @@ -188,10 +188,9 @@ test('firewall rule form targets table', async ({ page }) => { // now add a subnet by entering text await selectOption(page, 'Target type', 'VPC subnet') await subnetNameField.fill('abc') - await page.getByText('abc').first().click() - // hit enter to move focus to the add button + // hit enter to submit the subform + await subnetNameField.press('Enter') await subnetNameField.press('Enter') - await addButton.click() await expectRowVisible(targets, { Type: 'subnet', Value: 'abc' }) // add IP target From 32257bb914babcd1498754795c009174b2f1b5e9 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Mon, 11 Nov 2024 20:09:27 -0800 Subject: [PATCH 07/12] Use 'open' render prop from HeadlessUI to help with combobox interactivity --- app/components/form/fields/ComboboxField.tsx | 3 - app/forms/firewall-rules-common.tsx | 26 +-- app/ui/lib/Combobox.tsx | 198 ++++++++++--------- 3 files changed, 110 insertions(+), 117 deletions(-) diff --git a/app/components/form/fields/ComboboxField.tsx b/app/components/form/fields/ComboboxField.tsx index 6d782de640..a49561b7dd 100644 --- a/app/components/form/fields/ComboboxField.tsx +++ b/app/components/form/fields/ComboboxField.tsx @@ -32,7 +32,6 @@ export type ComboboxFieldProps< name: TName control: Control onChange?: (value: string | null | undefined) => void - onKeyDown?: (event: React.KeyboardEvent) => void validate?: Validate, TFieldValues> } & ComboboxBaseProps @@ -46,7 +45,6 @@ export function ComboboxField< label = capitalize(name), required, onChange, - onKeyDown, allowArbitraryValues, placeholder, // Intent is to not show both a placeholder and a description, while still having good defaults; prefer a description to a placeholder @@ -89,7 +87,6 @@ export function ComboboxField< onChange?.(value) setSelectedItemLabel(getSelectedLabelFromValue(items, value)) }} - onKeyDown={onKeyDown} allowArbitraryValues={allowArbitraryValues} inputRef={field.ref} {...props} diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 4dfe2a649a..6a16d8f177 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -6,7 +6,7 @@ * Copyright Oxide Computer Company */ -import { useEffect, useState, type ReactNode } from 'react' +import { useEffect, type ReactNode } from 'react' import { useController, useForm, type Control } from 'react-hook-form' import { @@ -105,8 +105,6 @@ const TargetAndHostFilterSubform = ({ data: { items: vpcSubnets }, } = usePrefetchedApiQuery('vpcSubnetList', { query: { project, vpc } }) - const [readyToSubmit, setReadyToSubmit] = useState(false) - const subform = useForm({ defaultValues: targetAndHostDefaultValues }) const field = useController({ name: `${sectionType}s`, control }).field @@ -118,7 +116,6 @@ const TargetAndHostFilterSubform = ({ if (field.value.some((f) => f.value === value && f.type === type)) return field.onChange([...field.value, { type, value }]) subform.reset(targetAndHostDefaultValues) - setReadyToSubmit(false) }) // HACK: we need to reset the target form completely after a successful submit, @@ -149,11 +146,9 @@ const TargetAndHostFilterSubform = ({ // back to validating on submit instead of change. Also resets readyToSubmit. const onTypeChange = () => { subform.reset({ type: subform.getValues('type'), value: '' }) - setReadyToSubmit(false) } const onInputChange = (value: string) => { subform.setValue('value', value) - setReadyToSubmit(false) } return ( @@ -185,21 +180,18 @@ const TargetAndHostFilterSubform = ({ {...getFilterValueProps(valueType)} description="Select an option or enter a custom value" control={subformControl} - onKeyDown={(e: React.KeyboardEvent) => { - if (e.key === KEYS.down || e.key === KEYS.up) { - setReadyToSubmit(false) - } - if (e.key === KEYS.enter && value.length) { - // the first time they hit the enter key, set readyToSubmit to true, - // so that the second time they hit enter, it submits the subform - if (!readyToSubmit) { - setReadyToSubmit(true) - } else { + onKeyDown={(e, open) => { + // When the combobox is closed, enter should submit + // the subform if there's some value in the field. + // Enter should not submit the overall form. + if (!open && e.key === KEYS.enter) { + if (value.length) { submitSubform(e) + } else { + e.preventDefault() } } }} - onChange={() => setReadyToSubmit(true)} onInputChange={onInputChange} items={items} allowArbitraryValues diff --git a/app/ui/lib/Combobox.tsx b/app/ui/lib/Combobox.tsx index ebf24e3ce1..25b49743b4 100644 --- a/app/ui/lib/Combobox.tsx +++ b/app/ui/lib/Combobox.tsx @@ -62,6 +62,7 @@ export type ComboboxBaseProps = { * For example, if a form value should be set when the user types, use `onInputChange`. */ onInputChange?: (value: string) => void + onKeyDown?: (event: React.KeyboardEvent, open?: boolean) => void } type ComboboxProps = { @@ -69,7 +70,6 @@ type ComboboxProps = { selectedItemLabel: string hasError?: boolean onChange: (value: string) => void - onKeyDown?: (event: React.KeyboardEvent) => void /** Necessary if you want RHF to be able to focus it on error */ inputRef?: Ref } & ComboboxBaseProps @@ -151,108 +151,112 @@ export const Combobox = ({ immediate {...props} > - {label && ( - // TODO: FieldLabel needs a real ID -

- - {label} - - {description && ( - {description} - )} -
- )} -
- - allowArbitraryValues ? selectedItemValue : selectedItemLabel - } - onChange={(event) => { - // updates the query state as the user types, in order to filter the list of items - setQuery(event.target.value) - // if the parent component wants to know about input changes, call the callback - onInputChange?.(event.target.value) - }} - onKeyDown={onKeyDown} - placeholder={placeholder} - disabled={disabled || isLoading} - className={cn( - `h-10 w-full rounded !border-none px-3 py-2 !outline-none text-sans-md text-default placeholder:text-quaternary`, - disabled - ? 'cursor-not-allowed text-disabled bg-disabled !border-default' - : 'bg-default', - hasError && 'focus-error' + {({ open }) => ( + <> + {label && ( + // TODO: FieldLabel needs a real ID +
+ + {label} + + {description && ( + {description} + )} +
)} - /> - {items.length > 0 && ( - - - - )} -
- {(items.length > 0 || allowArbitraryValues) && ( - - {filteredItems.map((item) => ( - + allowArbitraryValues ? selectedItemValue : selectedItemLabel + } + onChange={(event) => { + // updates the query state as the user types, in order to filter the list of items + setQuery(event.target.value) + // if the parent component wants to know about input changes, call the callback + onInputChange?.(event.target.value) + }} + onKeyDown={(e) => onKeyDown && onKeyDown(e, open)} + placeholder={placeholder} + disabled={disabled || isLoading} + className={cn( + `h-10 w-full rounded !border-none px-3 py-2 !outline-none text-sans-md text-default placeholder:text-quaternary`, + disabled + ? 'cursor-not-allowed text-disabled bg-disabled !border-default' + : 'bg-default', + hasError && 'focus-error' + )} + /> + {items.length > 0 && ( + + + + )} + + {(items.length > 0 || allowArbitraryValues) && ( + - {({ focus, selected }) => ( - // This *could* be done with data-[focus] and data-[selected] instead, but - // it would be a lot more verbose. those can only be used with TW classes, - // not our .is-selected and .is-highlighted, so we'd have to copy the pieces - // of those rules one by one. Better to rely on the shared classes. -
( + - {item.label} -
+ {({ focus, selected }) => ( + // This *could* be done with data-[focus] and data-[selected] instead, but + // it would be a lot more verbose. those can only be used with TW classes, + // not our .is-selected and .is-highlighted, so we'd have to copy the pieces + // of those rules one by one. Better to rely on the shared classes. +
+ {item.label} {open} +
+ )} + + ))} + {!allowArbitraryValues && filteredItems.length === 0 && ( + +
No items match
+ )} - - ))} - {!allowArbitraryValues && filteredItems.length === 0 && ( - -
No items match
- + )} - + )} From b0c7011caa8d5cffdcf308689fe3ff98b0e002bd Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Tue, 12 Nov 2024 09:33:04 -0800 Subject: [PATCH 08/12] refactor .length; start refactoring preventDefault --- app/forms/firewall-rules-common.tsx | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 6a16d8f177..cea3449985 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -185,10 +185,9 @@ const TargetAndHostFilterSubform = ({ // the subform if there's some value in the field. // Enter should not submit the overall form. if (!open && e.key === KEYS.enter) { - if (value.length) { + e.preventDefault() // prevent full form submission + if (value.length > 0) { submitSubform(e) - } else { - e.preventDefault() } } }} @@ -229,7 +228,7 @@ const TargetAndHostFilterSubform = ({ onClear={() => subform.reset()} onSubmit={submitSubform} /> - {!!field.value.length && ( + {field.value.length > 0 && ( - {!!ports.value.length && ( + {ports.value.length > 0 && ( Port ranges From 80d73b9f7a577225d25dd481507f58e1469c6539 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Tue, 12 Nov 2024 09:55:09 -0800 Subject: [PATCH 09/12] Add onEnter prop; move 'preventDefault when user presses Enter key' into Combobox component --- app/forms/firewall-rules-common.tsx | 12 ++---------- app/ui/lib/Combobox.tsx | 22 +++++++++++++++++++--- 2 files changed, 21 insertions(+), 13 deletions(-) diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index cea3449985..52b138cdea 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -180,16 +180,8 @@ const TargetAndHostFilterSubform = ({ {...getFilterValueProps(valueType)} description="Select an option or enter a custom value" control={subformControl} - onKeyDown={(e, open) => { - // When the combobox is closed, enter should submit - // the subform if there's some value in the field. - // Enter should not submit the overall form. - if (!open && e.key === KEYS.enter) { - e.preventDefault() // prevent full form submission - if (value.length > 0) { - submitSubform(e) - } - } + onEnter={(e) => { + if (value.length > 0) submitSubform(e) }} onInputChange={onInputChange} items={items} diff --git a/app/ui/lib/Combobox.tsx b/app/ui/lib/Combobox.tsx index 25b49743b4..ad0a4e1feb 100644 --- a/app/ui/lib/Combobox.tsx +++ b/app/ui/lib/Combobox.tsx @@ -62,13 +62,17 @@ export type ComboboxBaseProps = { * For example, if a form value should be set when the user types, use `onInputChange`. */ onInputChange?: (value: string) => void - onKeyDown?: (event: React.KeyboardEvent, open?: boolean) => void + /** Fires with any keypress in the Combobox input */ + onKeyDown?: (event: React.KeyboardEvent) => void + /** Fires whenever the Enter key is pressed while Combobox input has focus */ + onEnter?: (event: React.KeyboardEvent) => void } type ComboboxProps = { selectedItemValue: string selectedItemLabel: string hasError?: boolean + /** Fires when the user *selects* an item from the list */ onChange: (value: string) => void /** Necessary if you want RHF to be able to focus it on error */ inputRef?: Ref @@ -87,6 +91,7 @@ export const Combobox = ({ isLoading, onChange, onKeyDown, + onEnter, onInputChange, allowArbitraryValues = false, hideOptionalTag, @@ -138,6 +143,17 @@ export const Combobox = ({ } const zIndex = usePopoverZIndex() const id = useId() + const handleKeyDown = (e: React.KeyboardEvent, open: boolean) => { + // Prevent form submission when the user presses Enter inside a combobox. + // The combobox component already handles Enter keypresses to select items, + // so we only preventDefault when the combobox is closed. + if (e.key === 'Enter' && !open) { + e.preventDefault() + onEnter?.(e) + } + onKeyDown?.(e) + } + return ( <> onKeyDown && onKeyDown(e, open)} + onKeyDown={(e) => handleKeyDown(e, open)} placeholder={placeholder} disabled={disabled || isLoading} className={cn( @@ -244,7 +260,7 @@ export const Combobox = ({ 'is-highlighted': focus, })} > - {item.label} {open} + {item.label} )} From 7caf41a4550ee2ca3886d40d7393b0d022a51ece Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Tue, 12 Nov 2024 09:58:39 -0800 Subject: [PATCH 10/12] Add ALL_ISH limit to subnet fetching --- app/forms/firewall-rules-common.tsx | 2 +- app/forms/firewall-rules-create.tsx | 4 +++- app/forms/firewall-rules-edit.tsx | 4 +++- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 52b138cdea..baf659c69f 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -103,7 +103,7 @@ const TargetAndHostFilterSubform = ({ } = usePrefetchedApiQuery('vpcList', { query: { project, limit: ALL_ISH } }) const { data: { items: vpcSubnets }, - } = usePrefetchedApiQuery('vpcSubnetList', { query: { project, vpc } }) + } = usePrefetchedApiQuery('vpcSubnetList', { query: { project, vpc, limit: ALL_ISH } }) const subform = useForm({ defaultValues: targetAndHostDefaultValues }) const field = useController({ name: `${sectionType}s`, control }).field diff --git a/app/forms/firewall-rules-create.tsx b/app/forms/firewall-rules-create.tsx index 35aee97230..01f14317b2 100644 --- a/app/forms/firewall-rules-create.tsx +++ b/app/forms/firewall-rules-create.tsx @@ -61,7 +61,9 @@ CreateFirewallRuleForm.loader = async ({ params }: LoaderFunctionArgs) => { apiQueryClient.prefetchQuery('vpcFirewallRulesView', { query: { project, vpc } }), apiQueryClient.prefetchQuery('instanceList', { query: { project, limit: ALL_ISH } }), apiQueryClient.prefetchQuery('vpcList', { query: { project, limit: ALL_ISH } }), - apiQueryClient.prefetchQuery('vpcSubnetList', { query: { project, vpc } }), + apiQueryClient.prefetchQuery('vpcSubnetList', { + query: { project, vpc, limit: ALL_ISH }, + }), ]) return null diff --git a/app/forms/firewall-rules-edit.tsx b/app/forms/firewall-rules-edit.tsx index bbea4f975e..b9bd16fa9d 100644 --- a/app/forms/firewall-rules-edit.tsx +++ b/app/forms/firewall-rules-edit.tsx @@ -39,7 +39,9 @@ EditFirewallRuleForm.loader = async ({ params }: LoaderFunctionArgs) => { apiQueryClient.fetchQuery('vpcFirewallRulesView', { query: { project, vpc } }), apiQueryClient.prefetchQuery('instanceList', { query: { project, limit: ALL_ISH } }), apiQueryClient.prefetchQuery('vpcList', { query: { project, limit: ALL_ISH } }), - apiQueryClient.prefetchQuery('vpcSubnetList', { query: { project, vpc } }), + apiQueryClient.prefetchQuery('vpcSubnetList', { + query: { project, vpc, limit: ALL_ISH }, + }), ]) const originalRule = firewallRules.rules.find((r) => r.name === rule) From 2b09b723d28a57cc061dcb697c92185eea889761 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Tue, 12 Nov 2024 12:28:17 -0800 Subject: [PATCH 11/12] refactor props, inline functions, make things better --- app/forms/firewall-rules-common.tsx | 4 +- app/ui/lib/Combobox.tsx | 244 ++++++++++++++-------------- 2 files changed, 119 insertions(+), 129 deletions(-) diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index baf659c69f..43d290b630 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -180,9 +180,7 @@ const TargetAndHostFilterSubform = ({ {...getFilterValueProps(valueType)} description="Select an option or enter a custom value" control={subformControl} - onEnter={(e) => { - if (value.length > 0) submitSubform(e) - }} + onEnter={submitSubform} onInputChange={onInputChange} items={items} allowArbitraryValues diff --git a/app/ui/lib/Combobox.tsx b/app/ui/lib/Combobox.tsx index ad0a4e1feb..05523b7054 100644 --- a/app/ui/lib/Combobox.tsx +++ b/app/ui/lib/Combobox.tsx @@ -62,8 +62,6 @@ export type ComboboxBaseProps = { * For example, if a form value should be set when the user types, use `onInputChange`. */ onInputChange?: (value: string) => void - /** Fires with any keypress in the Combobox input */ - onKeyDown?: (event: React.KeyboardEvent) => void /** Fires whenever the Enter key is pressed while Combobox input has focus */ onEnter?: (event: React.KeyboardEvent) => void } @@ -90,7 +88,6 @@ export const Combobox = ({ disabled, isLoading, onChange, - onKeyDown, onEnter, onInputChange, allowArbitraryValues = false, @@ -143,138 +140,133 @@ export const Combobox = ({ } const zIndex = usePopoverZIndex() const id = useId() - const handleKeyDown = (e: React.KeyboardEvent, open: boolean) => { - // Prevent form submission when the user presses Enter inside a combobox. - // The combobox component already handles Enter keypresses to select items, - // so we only preventDefault when the combobox is closed. - if (e.key === 'Enter' && !open) { - e.preventDefault() - onEnter?.(e) - } - onKeyDown?.(e) - } - return ( - <> - onChange(val || '')} - // we only want to keep the query on close when arbitrary values are allowed - onClose={allowArbitraryValues ? undefined : () => setQuery('')} - disabled={disabled || isLoading} - immediate - {...props} - > - {({ open }) => ( - <> - {label && ( - // TODO: FieldLabel needs a real ID -
- - {label} - - {description && ( - {description} - )} -
+ onChange(val || '')} + // we only want to keep the query on close when arbitrary values are allowed + onClose={allowArbitraryValues ? undefined : () => setQuery('')} + disabled={disabled || isLoading} + immediate + {...props} + > + {({ open }) => ( + <> + {label && ( + // TODO: FieldLabel needs a real ID +
+ + {label} + + {description && ( + {description} + )} +
+ )} +
+ + allowArbitraryValues ? selectedItemValue : selectedItemLabel + } + onChange={(event) => { + // updates the query state as the user types, in order to filter the list of items + setQuery(event.target.value) + // if the parent component wants to know about input changes, call the callback + onInputChange?.(event.target.value) + }} + onKeyDown={(e) => { + // Prevent form submission when the user presses Enter inside a combobox. + // The combobox component already handles Enter keypresses to select items, + // so we only preventDefault when the combobox is closed. + if (e.key === 'Enter' && !open) { + e.preventDefault() + onEnter?.(e) + } + }} + placeholder={placeholder} + disabled={disabled || isLoading} className={cn( - `flex rounded border focus-within:ring-2`, - hasError - ? 'focus-error border-error-secondary focus-within:ring-error-secondary hover:border-error' - : 'border-default focus-within:ring-accent-secondary hover:border-hover', + `h-10 w-full rounded !border-none px-3 py-2 !outline-none text-sans-md text-default placeholder:text-quaternary`, disabled ? 'cursor-not-allowed text-disabled bg-disabled !border-default' : 'bg-default', - disabled && hasError && '!border-error-secondary' + hasError && 'focus-error' )} - // Putting the inputRef on the div makes it so the div can be focused by RHF when there's an error. - // We want to focus on the div (rather than the input) so the combobox doesn't open automatically - // and obscure the error message. - ref={inputRef} - // tabIndex=-1 is necessary to make the div focusable - tabIndex={-1} + /> + {items.length > 0 && ( + + + + )} +
+ {(items.length > 0 || allowArbitraryValues) && ( + - - allowArbitraryValues ? selectedItemValue : selectedItemLabel - } - onChange={(event) => { - // updates the query state as the user types, in order to filter the list of items - setQuery(event.target.value) - // if the parent component wants to know about input changes, call the callback - onInputChange?.(event.target.value) - }} - onKeyDown={(e) => handleKeyDown(e, open)} - placeholder={placeholder} - disabled={disabled || isLoading} - className={cn( - `h-10 w-full rounded !border-none px-3 py-2 !outline-none text-sans-md text-default placeholder:text-quaternary`, - disabled - ? 'cursor-not-allowed text-disabled bg-disabled !border-default' - : 'bg-default', - hasError && 'focus-error' - )} - /> - {items.length > 0 && ( - ( + - - + {({ focus, selected }) => ( + // This *could* be done with data-[focus] and data-[selected] instead, but + // it would be a lot more verbose. those can only be used with TW classes, + // not our .is-selected and .is-highlighted, so we'd have to copy the pieces + // of those rules one by one. Better to rely on the shared classes. +
+ {item.label} +
+ )} + + ))} + {!allowArbitraryValues && filteredItems.length === 0 && ( + +
No items match
+ )} - - {(items.length > 0 || allowArbitraryValues) && ( - - {filteredItems.map((item) => ( - - {({ focus, selected }) => ( - // This *could* be done with data-[focus] and data-[selected] instead, but - // it would be a lot more verbose. those can only be used with TW classes, - // not our .is-selected and .is-highlighted, so we'd have to copy the pieces - // of those rules one by one. Better to rely on the shared classes. -
- {item.label} -
- )} - - ))} - {!allowArbitraryValues && filteredItems.length === 0 && ( - -
No items match
- - )} - - )} - - )} - - + + )} + + )} + ) } From 1c3108c2d70024c45160bc47620a3032682ad438 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Tue, 12 Nov 2024 13:02:23 -0800 Subject: [PATCH 12/12] clean up comments --- app/forms/firewall-rules-common.tsx | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 43d290b630..2d31a3504e 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -109,11 +109,8 @@ const TargetAndHostFilterSubform = ({ const field = useController({ name: `${sectionType}s`, control }).field const submitSubform = subform.handleSubmit(({ type, value }) => { - // TODO: do this with a normal validation - // ignore click if empty or a duplicate - // TODO: show error instead of ignoring click if (!type || !value) return - if (field.value.some((f) => f.value === value && f.type === type)) return + if (field.value.some((f) => f.type === type && f.value === value)) return field.onChange([...field.value, { type, value }]) subform.reset(targetAndHostDefaultValues) })