-
Notifications
You must be signed in to change notification settings - Fork 13
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.
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
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Thank you! We will take a look soon. |
isLoading={areImagesLoading} | ||
items={images.map((i) => toImageComboboxItem(i, true))} | ||
required | ||
onInputChange={() => { | ||
diskImageIdField.onChange() | ||
}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
app/forms/disk-create.tsx
Outdated
control={control} | ||
name="diskSource.imageId" | ||
label="Source image" | ||
placeholder="Select an image" | ||
placeholder="Select an image or enter an image name" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the original placeholder was sufficient in both cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say that this placeholder would signal the user in a better way that they could search for a snapshot/image as opposed of just having the presence of the text cursor in the field.
It's worth noting that I'm not very familiarized with Oxide's userbase, so this comes from thin air :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might revisit placeholder copy in the future, but for now would want to keep the shorter version from before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 19621a5.
This replaces the Listbox in snapshot and image source selections with a Combobox. Solving (hopefully) #2827.
combobox.mp4