Skip to content

Replace Listbox in Create Disk side modal with Combobox #2829

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Replace Listbox in Create Disk side modal with Combobox #2829

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
48a317d
Replace Listbox in Create Disk side modal with Combobox
septum May 20, 2025
b3a0a7d
Update e2e tests to account for the comboboxes
septum May 20, 2025
a01a795
Fix issues raised by CI pipeline
septum May 22, 2025
7f6b651
Fix issues stated in code review
septum May 23, 2025
19621a5
Revert changes in placeholders
septum May 29, 2025
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
21 changes: 16 additions & 5 deletions app/forms/disk-create.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,10 @@ import {
type Image,
} from '@oxide/api'

import { ComboboxField } from '~/components/form/fields/ComboboxField'
import { DescriptionField } from '~/components/form/fields/DescriptionField'
import { DiskSizeField } from '~/components/form/fields/DiskSizeField'
import { toImageComboboxItem } from '~/components/form/fields/ImageSelectField'
import { ListboxField } from '~/components/form/fields/ListboxField'
import { NameField } from '~/components/form/fields/NameField'
import { RadioField } from '~/components/form/fields/RadioField'
import { SideModalForm } from '~/components/form/SideModalForm'
Expand Down Expand Up @@ -171,6 +171,7 @@ const DiskSourceField = ({
field: { value, onChange },
} = useController({ control, name: 'diskSource' })
const diskSizeField = useController({ control, name: 'size' }).field
const diskImageIdField = useController({ control, name: 'diskSource.imageId' }).field

return (
<>
Expand Down Expand Up @@ -210,14 +211,17 @@ const DiskSourceField = ({
/>
)}
{value.type === 'image' && (
<ListboxField
<ComboboxField
control={control}
name="diskSource.imageId"
label="Source image"
placeholder="Select an image"
placeholder="Select an image or enter an image name"
isLoading={areImagesLoading}
items={images.map((i) => toImageComboboxItem(i, true))}
required
onInputChange={() => {
diskImageIdField.onChange()
}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does this do?

Copy link
Author

Choose a reason for hiding this comment

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

It allows the modification of the underlying text value after the selection of one item, otherwise, the text value changes only by selecting another item in the list and not by input events.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, and good catch. I think you can move this fix to the ComboboxField component by adding an onInputChange={() => field.onChange()} to the main Combobox props, fixing it for all Comboboxes.

It does seem like there's a funny interaction where selecting an option, then deleting some of the contents, then blurring the field by clicking outside the component then erases the selected option. Would there be a way to prevent that selected option from getting lost?

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good, I'll prepare a change for this.

For the first item: I'm guessing we should destructure onInputChange from the ComboboxField props and call it in a similar fashion as onChange (e.g. onInputChange?.(value)), right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for dropping this; yes, good call! Adding onInput Change to the ComboboxField props and calling something like

onInputChange={(value) => {
  field.onChange()
  onInputChange?.(value)
}}

makes sense to me so we don't swallow anything passed through. Is that about what you were thinking?

onChange={(id) => {
const image = images.find((i) => i.id === id)! // if it's selected, it must be present
const imageSizeGiB = image.size / GiB
Expand Down Expand Up @@ -252,13 +256,17 @@ const SnapshotSelectField = ({ control }: { control: Control<DiskCreate> }) => {

const snapshots = snapshotsQuery.data?.items || []
const diskSizeField = useController({ control, name: 'size' }).field
const diskSnapshotIdField = useController({
control,
name: 'diskSource.snapshotId',
}).field

return (
<ListboxField
<ComboboxField
control={control}
name="diskSource.snapshotId"
label="Source snapshot"
placeholder="Select a snapshot"
placeholder="Select a snapshot or enter a snapshot name"
items={snapshots.map((i) => {
const formattedSize = filesize(i.size, { base: 2, output: 'object' })
return {
Expand All @@ -278,6 +286,9 @@ const SnapshotSelectField = ({ control }: { control: Control<DiskCreate> }) => {
})}
isLoading={snapshotsQuery.isPending}
required
onInputChange={() => {
diskSnapshotIdField.onChange()
}}
onChange={(id) => {
const snapshot = snapshots.find((i) => i.id === id)! // if it's selected, it must be present
const snapshotSizeGiB = snapshot.size / GiB
Expand Down
12 changes: 9 additions & 3 deletions test/e2e/disks.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,20 +80,26 @@ test.describe('Disk create', () => {

test('from snapshot', async ({ page }) => {
await page.getByRole('radio', { name: 'Snapshot' }).click()
await page.getByRole('button', { name: 'Source snapshot' }).click()
await page
.getByPlaceholder('Select a snapshot or enter a snapshot name', { exact: true })
.click()
await page.getByRole('option', { name: 'delete-500' }).click()
})

// max-size snapshot required a fix
test('from max-size snapshot', async ({ page }) => {
await page.getByRole('radio', { name: 'Snapshot' }).click()
await page.getByRole('button', { name: 'Source snapshot' }).click()
await page
.getByPlaceholder('Select a snapshot or enter a snapshot name', { exact: true })
.click()
await page.getByRole('option', { name: 'snapshot-max' }).click()
})

test('from image', async ({ page }) => {
await page.getByRole('radio', { name: 'Image' }).click()
await page.getByRole('button', { name: 'Source image' }).click()
await page
.getByPlaceholder('Select an image or enter an image name', { exact: true })
.click()
await page.getByRole('option', { name: 'image-3' }).click()
})

Expand Down
4 changes: 3 additions & 1 deletion test/e2e/instance-disks.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,9 @@ test('Create disk', async ({ page }) => {
await expect(createForm.getByRole('textbox', { name: 'Description' })).toBeVisible()

await page.getByRole('radio', { name: 'Snapshot' }).click()
await page.getByRole('button', { name: 'Source snapshot' }).click()
await page
.getByPlaceholder('Select a snapshot or enter a snapshot name', { exact: true })
.click()
await page.getByRole('option', { name: 'snapshot-heavy' }).click()

await createForm.getByRole('button', { name: 'Create disk' }).click()
Expand Down
Loading