Attach disk modal + ModalForm

[benjaminleonard]

Attach disk has only one item, would work better as a regular modal. This does that and adds a complementary ModalForm component to match the SideModalForm.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
console	✅ Ready (Inspect)	Visit Preview	Apr 16, 2025 4:22pm

[david-crespo]

Nice. Would like to see at least one existing one converted to see how much fiddliness we're saving. Don't necessarily need to convert them all, but it's probably not too hard.

[david-crespo]

This comment has been wrong for two years 😁

https://github.com/oxidecomputer/console/blame/eed4a4e7873e1d7511cfe66c7ed9f16e916f2ae1/app/components/form/SideModalForm.tsx#L33-L41

[david-crespo]

This is pretty satisfying. Feels a lot harder to mess up.

[david-crespo]

One thing to keep an eye out for is that there is at least one of these modal forms that isn't supposed to have a <form> element at all because it's inside another form. I see the stopPropagation thing inside onSubmit is supposed to fix that (nice comment, past me), so maybe we're good, and the problem when I tried it came from using the <form> directly without stopPropagation.

Strictly speaking, form inside form is Not Allowed, but it's entirely possible it's fine as long as we mitigate this particular issue.

Edit: I think the above is true, but ONLY because we are rendering the form in a modal in a portal, which means from the DOM's point of view it is not actually nested. The stopPropagation is still needed to prevent events from bubbling up through the portal, which they can do for some reason.

[david-crespo]

Disk attach is opening with its tongue out on both instance create and instance disks tab. Can't have that.

2025-03-05-model-tongue-out.mp4

[benjaminleonard]

Edit: I think the above is true, but ONLY we are rendering the form in a modal in a portal, which means from the DOM's point of view it is not actually nested. The stopPropagation is still needed to prevent events from bubbling up through the portal, which they can do for some reason.

Yeah I tried without stopPropagation initially, and the attach disk modal would submit the entire form – so definitely still necessary here too.

Disk attach is opening with its tongue out on both instance create and instance disks tab. Can't have that.

Hmm, it does on the side pane also but less obvious because its not sticking out. Since it automatically focuses the first input and focusing options the listbox. Not sure what the solution is here? Perhaps it shouldn't be opening on focus, and instead if the user presses a key when its focused (or clicks ofc). Should explore this further.

[benjaminleonard]

Do we want to think about our approach to labels here? As in do we use the full version as it is, or just use the verb for. Aka "attach", "create", "save"

[david-crespo]

I think just the verb, otherwise it's too long.

[david-crespo]

On second thought I like the nouns. I'll try this as Attach IP to make it shorter.

[benjaminleonard]

On the SideModelForm and FullPageForm we let the user pass in the buttons directly. This way is more foolproof, but maybe worth considering unifying our approach.

[david-crespo]

This all looks good, I'm about ready to merge. I was playing around with the modal opening with the combobox focused, and I found that by moving the close button back to being the first thing, we don't have the problem, and I don't see the distracting focus ring on the close button (which I vaguely remember was the main reason we didn't want it focusing by default). Seems ok?

[benjaminleonard]

This would be a departure from the side modal – and I think it's generally helpful to hit the first input on open. Might be worth prodding at the listbox to see if there's a way we can require input or the user to press enter to open?

[benjaminleonard]

Thinking of this behaviour:

https://headlessui.com/react/combobox

[david-crespo]

Oh, very good point. So we are using the immediate prop, which causes this behavior.

https://headlessui.com/react/combobox#opening-the-dropdown-on-focus

console/app/ui/lib/Combobox.tsx

Lines 149 to 159 in 1d8c3b7

	<HCombobox
	// necessary, as the displayed "value" is not the same as the actual selected item's *value*
	value={selectedItemValue}
	// fallback to '' allows clearing field to work
	onChange={(val) => 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}
	>

Turning it off produces the below behavior. The options don't show on focus — they only show up when you either type something or click the button. Not sure how I feel about it. I might just be used to the current behavior. It probably says something that immediate=false is the default.

2025-04-18-listbox-non-immediate.mp4

[david-crespo]

Yeah, it feels kinda sad to me to click on a combobox and not see the things listed, and it doesn't feel very intuitive that you have to start typing when you might not know what you're looking for. Clicking the button doesn't feel that intuitive either. Between that and the things being open by default, I think I would choose the latter. You should try it — all you have to do is comment out immediate in Combobox.tsx.