Skip to content

Commit

Permalink
Virtual list entry for arbitrary values in comboboxes (#2518)
Browse files Browse the repository at this point in the history 
* Update how allowArbitraryValue functions in comboboxes

* Update vertical position, to reduce peek-through of button on lower layer

* update test to work with new approach

* Update tests, though Safari is still a bit touchy

* Select via Enter

* Update test

* Change UX for allowArbitraryOption

* Update test

* More test updating

* new copy: 'Use custom value:'

* gray out "custom:" in virtual item, fix query clearing onClose, e2e test

* use key to nuke combobox on host filter value type change

* explicit useEffect for clearing query whenever value is cleared

---------

Co-authored-by: David Crespo <[email protected]>
  • Loading branch information
@charliepark @david-crespo
charliepark and david-crespo authored Nov 7, 2024
1 parent 7dcd41c commit 6f83d41
Show file tree
Hide file tree
Showing 3 changed files with 110 additions and 19 deletions.
2 changes: 1 addition & 1 deletion .eslintrc.cjs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ module.exports = {
rules: {
'playwright/expect-expect': [
'warn',
{ assertFunctionNames: ['expectVisible', 'expectRowVisible'] },
{ assertFunctionNames: ['expectVisible', 'expectRowVisible', 'expectOptions'] },
],
},
},
Expand Down
58 changes: 48 additions & 10 deletions app/ui/lib/Combobox.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import {
} from '@headlessui/react'
import cn from 'classnames'
import { matchSorter } from 'match-sorter'
import { useId, useState, type ReactNode, type Ref } from 'react'
import { useEffect, useId, useState, type ReactNode, type Ref } from 'react'

import { SelectArrows6Icon } from '@oxide/design-system/icons/react'

Expand Down Expand Up @@ -97,6 +97,43 @@ export const Combobox = ({
keys: ['selectedLabel', 'label'],
sorter: (items) => items, // preserve original order, don't sort by match
})

// In the arbitraryValues case, clear the query whenever the value is cleared.
// this is necessary, e.g., for the firewall rules form when you submit the
// targets subform and clear the field. Two possible changes we might want to make
// here if we run into issues:
//
// 1. do it all the time, not just in the arbitraryValues case
// 2. do it on all value changes, not just on clear
//
// Currently, I don't think there are any arbitraryValues=false cases where we
// set the value from outside. There is an arbitraryvalues=true case where we
// setValue to something other than empty string, but we don't need the
// sync because that setValue is done in onInputChange and we already are
// doing setQuery in here along with it.
useEffect(() => {
if (allowArbitraryValues && !selectedItemValue) {
setQuery('')
}
}, [allowArbitraryValues, selectedItemValue])

// If the user has typed in a value that isn't in the list,
// add it as an option if `allowArbitraryValues` is true
if (
allowArbitraryValues &&
query.length > 0 &&
!filteredItems.some((i) => i.selectedLabel === query)
) {
filteredItems.push({
value: query,
label: (
<>
<span className="text-secondary">Custom:</span> {query}
</>
),
selectedLabel: query,
})
}
const zIndex = usePopoverZIndex()
const id = useId()
return (
Expand All @@ -106,7 +143,8 @@ export const Combobox = ({
value={selectedItemValue}
// fallback to '' allows clearing field to work
onChange={(val) => onChange(val || '')}
onClose={() => setQuery('')}
// we only want to keep the query on close when arbitrary values are allowed
onClose={allowArbitraryValues ? undefined : () => setQuery('')}
disabled={disabled || isLoading}
immediate
{...props}
Expand Down Expand Up @@ -177,18 +215,13 @@ export const Combobox = ({
</ComboboxButton>
)}
</div>
{items.length > 0 && (
{(items.length > 0 || allowArbitraryValues) && (
<ComboboxOptions
anchor="bottom start"
// 14px gap is presumably because it's measured from inside the outline or something
className={`ox-menu pointer-events-auto ${zIndex} relative w-[calc(var(--input-width)+var(--button-width))] overflow-y-auto border !outline-none border-secondary [--anchor-gap:14px] empty:hidden`}
className={`ox-menu pointer-events-auto ${zIndex} relative w-[calc(var(--input-width)+var(--button-width))] overflow-y-auto border !outline-none border-secondary [--anchor-gap:13px] empty:hidden`}
modal={false}
>
{!allowArbitraryValues && filteredItems.length === 0 && (
<ComboboxOption disabled value="no-matches" className="relative">
<div className="ox-menu-item !text-disabled">No items match</div>
</ComboboxOption>
)}
{filteredItems.map((item) => (
<ComboboxOption
key={item.value}
Expand All @@ -202,7 +235,7 @@ export const Combobox = ({
// of those rules one by one. Better to rely on the shared classes.
<div
className={cn('ox-menu-item', {
'is-selected': selected,
'is-selected': selected && query !== item.value,
'is-highlighted': focus,
})}
>
Expand All @@ -211,6 +244,11 @@ export const Combobox = ({
)}
</ComboboxOption>
))}
{!allowArbitraryValues && filteredItems.length === 0 && (
<ComboboxOption disabled value="no-matches" className="relative">
<div className="ox-menu-item !text-disabled">No items match</div>
</ComboboxOption>
)}
</ComboboxOptions>
)}
</HCombobox>
Expand Down
69 changes: 61 additions & 8 deletions test/e2e/firewall-rules.e2e.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,9 @@
* Copyright Oxide Computer Company
*/

import {
clickRowAction,
expect,
expectRowVisible,
selectOption,
test,
type Locator,
} from './utils'
import { expect, test, type Locator, type Page } from '@playwright/test'

import { clickRowAction, expectRowVisible, selectOption } from './utils'

const defaultRules = ['allow-internal-inbound', 'allow-ssh', 'allow-icmp']

Expand Down Expand Up @@ -53,6 +48,7 @@ test('can create firewall rule', async ({ page }) => {
// add host filter instance "host-filter-instance"
await selectOption(page, 'Host type', 'Instance')
await page.getByRole('combobox', { name: 'Instance name' }).fill('host-filter-instance')
await page.getByText('host-filter-instance').click()
await page.getByRole('button', { name: 'Add host filter' }).click()

// host is added to hosts table
Expand Down Expand Up @@ -167,11 +163,13 @@ test('firewall rule form targets table', async ({ page }) => {
// add targets with overlapping names and types to test delete

await targetVpcNameField.fill('abc')
await targetVpcNameField.press('Enter')
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
await targetVpcNameField.fill('mock-subnet')
await targetVpcNameField.press('Enter')
await addButton.click()
await expectRowVisible(targets, { Type: 'vpc', Value: 'mock-subnet' })

Expand All @@ -188,6 +186,7 @@ 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()
await addButton.click()
await expectRowVisible(targets, { Type: 'subnet', Value: 'abc' })

Expand Down Expand Up @@ -221,6 +220,7 @@ test('firewall rule form target validation', async ({ page }) => {
// Enter invalid VPC name
const vpcNameField = page.getByRole('combobox', { name: 'VPC name' }).first()
await vpcNameField.fill('ab-')
await vpcNameField.press('Enter')
await addButton.click()
await expect(nameError).toBeVisible()

Expand All @@ -246,6 +246,7 @@ test('firewall rule form target validation', async ({ page }) => {
await expect(ipError).toBeHidden()
await expect(nameError).toBeHidden()
await vpcNameField.fill('abc')
await page.getByText('abc').click()
await addButton.click()
await expectRowVisible(targets, { Type: 'vpc', Value: 'abc' })

Expand Down Expand Up @@ -284,6 +285,7 @@ test('firewall rule form host validation', async ({ page }) => {
// Enter invalid VPC name
const vpcNameField = page.getByRole('combobox', { name: 'VPC name' }).nth(1)
await vpcNameField.fill('ab-')
await vpcNameField.press('Enter')
await addButton.click()
await expect(nameError).toBeVisible()

Expand All @@ -309,6 +311,7 @@ test('firewall rule form host validation', async ({ page }) => {
await expect(ipError).toBeHidden()
await expect(nameError).toBeHidden()
await vpcNameField.fill('abc')
await page.getByText('abc').click()
await addButton.click()
await expectRowVisible(hosts, { Type: 'vpc', Value: 'abc' })

Expand Down Expand Up @@ -350,10 +353,12 @@ test('firewall rule form hosts table', async ({ page }) => {
// add hosts with overlapping names and types to test delete

await hostFiltersVpcNameField.fill('abc')
await hostFiltersVpcNameField.press('Enter')
await addButton.click()
await expectRowVisible(hosts, { Type: 'vpc', Value: 'abc' })

await hostFiltersVpcNameField.fill('def')
await hostFiltersVpcNameField.press('Enter')
await addButton.click()
await expectRowVisible(hosts, { Type: 'vpc', Value: 'def' })

Expand All @@ -364,6 +369,7 @@ test('firewall rule form hosts table', async ({ page }) => {

await selectOption(page, 'Host type', 'VPC subnet')
await page.getByRole('combobox', { name: 'Subnet name' }).fill('abc')
await page.getByRole('combobox', { name: 'Subnet name' }).press('Enter')
await addButton.click()
await expectRowVisible(hosts, { Type: 'subnet', Value: 'abc' })

Expand Down Expand Up @@ -427,6 +433,7 @@ test('can update firewall rule', async ({ page }) => {
// add host filter
await selectOption(page, 'Host type', 'VPC subnet')
await page.getByRole('combobox', { name: 'Subnet name' }).fill('edit-filter-subnet')
await page.getByText('edit-filter-subnet').click()
await page.getByRole('button', { name: 'Add host filter' }).click()

// new host is added to hosts table
Expand Down Expand Up @@ -561,3 +568,49 @@ test('name conflict error on edit', async ({ page }) => {
await page.getByRole('button', { name: 'Update rule' }).click()
await expectRowVisible(page.getByRole('table'), { Name: 'allow-icmp2', Priority: '37' })
})

async function expectOptions(page: Page, options: string[]) {
const selector = page.getByRole('option')
await expect(selector).toHaveCount(options.length)
for (const option of options) {
await expect(page.getByRole('option', { name: option })).toBeVisible()
}
}

test('arbitrary values combobox', async ({ page }) => {
await page.goto('/projects/mock-project/vpcs/mock-vpc/firewall-rules-new')

// test for bug where we'd persist the d after add and only show 'Custom: d'
const vpcInput = page.getByRole('combobox', { name: 'VPC name' }).first()
await vpcInput.focus()
await expectOptions(page, ['mock-vpc'])

await vpcInput.fill('d')
await expectOptions(page, ['Custom: d'])

await vpcInput.blur()
page.getByRole('button', { name: 'Add target' }).click()
await expect(vpcInput).toHaveValue('')

await vpcInput.focus()
await expectOptions(page, ['mock-vpc']) // bug cause failure here

// test keeping query around on blur
await selectOption(page, 'Target type', 'Instance')
const input = page.getByRole('combobox', { name: 'Instance name' })

await input.focus()
await expectOptions(page, ['db1', 'you-fail', 'not-there-yet'])

await input.fill('d')
await expectOptions(page, ['db1', 'Custom: d'])

await input.blur()
await expect(page.getByRole('option')).toBeHidden()

await expect(input).toHaveValue('d')
await input.focus()

// same options show up after blur (there was a bug around this)
await expectOptions(page, ['db1', 'Custom: d'])
})

0 comments on commit 6f83d41

Please sign in to comment.