-
Notifications
You must be signed in to change notification settings - Fork 18
Disk details side modal #2992
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
Disk details side modal #2992
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| isOpen | ||
| onDismiss={onDismiss} | ||
| animate={animate} | ||
| title="Disk details" |
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 prefer to call this Disk, but it looks stupid. The other modals like this (project image, silo image, identity provider) are all naturally two-word titles. @benjaminleonard @charliepark if you think it looks fine as Disk, I'm fine changing it.
| await page.getByRole('table').click() // focus the content pane | ||
| await page.mouse.wheel(0, 200) | ||
| // scroll so the dropdown menu isn't behind the pagination bar | ||
| await row.scrollIntoViewIfNeeded() |
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.
Claude figured out this test was breaking with an unwanted nav to disk detail because the click on the table was landing right in the middle of the table, on a disk link cell!
|
Back to the usual Firefox flake. Nature is healing. |
| return <span className="text-default">{data.data.name}</span> | ||
| return ( | ||
| <LinkCell to={pb.disk({ project, disk: data.data.name })}>{data.data.name}</LinkCell> | ||
| ) |
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.
Since the details view is just a side modal, we could theoretically just open it right on the snapshots page and it would work fine. The only problem with that is that it wouldn't be able to have a URL, which is probably also fine. I feel a little weird about there being two places to see disk details. On the other hand the current interaction is kind of jarring. It feels like a long way to go to nav to the disks page with a side modal open.
|
It's more or less okay, but I want to prod at the design a little. Solo properties table looks funky. Are you in a rush to merge? |
|
Not a big rush, would be nice if it was today. I want to get it into #2984. |
|
This got much bigger. I pulled out a read-only side modal form component. And used it in all of the read-only forms, which includes SSH key edit, Gateway edit, Image edit, and Identity Provider edit. This is nice because it makes these behave more consistently: they all get a footer with a close button and they all use the I also decided to make the disk detail modal for snapshots open inline on the snapshots page instead of navigating to the disks list page with the side modal open. It feels way better. 2025-12-18-snapshot-disk-modal.mp4 |
| <form | ||
| id={id} | ||
| className="ox-form is-side-modal" | ||
| className="ox-form" |
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.
The is-side-modal styling mechanism was removed in #2815
This change adds disk detail side modals to the disk list page and the instance disks tab. The one on the disk list gets its own route, but since the one on instances is already nested under the storage tab, I decided it's probably not worth giving it a path. I experimented with
?disk=xyzin the query string, but it feels a bit pointless and complicates the code.The motivation for this is that while working on local disks in #2984, I talked to @jmpesp and we agreed it is hard to make sense of the error that you can't attach a local disk associated with one sled to an instance with local disks on another sled. To help the user understand and deal with this, we want to put sled ID in the local disk response. Without this detail modal, there would be no place to display that sled ID.
The contents of the modal will have to be updated in #2984. I just wanted to keep the bulk of this change outside of that PR.
While working on this I noticed that the
AGENTS.mdwas too long and also out of date in some ways, so I cleaned that up too.