Add source cell for disks, with side modal#3214
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
The test failures were the first hour of the month UTC bug fixed in #3274. No need to merge main, re-running should work. |
|
I think you should consolidate the image side modals on the newer non-form version, like you said. I think we did the old one as a pseudo-form in anticipation of it becoming editable, and that's not really on the way. |
|
Pushed some small fixes based on a Fable review. The only one that's more than a few lines is d63b3d1, which pulls out a shared I increased the gap here from 1 to 1.5: 
I also added 
|
| const idCell = propertiesTableValue(modal, 'ID') | ||
| await expect(idCell.getByLabel('7f2309a5-13e3-47e0-8a4c-2a3b3bc992fd')).toBeVisible() | ||
| await expect(idCell.getByRole('button', { name: 'Click to copy' })).toBeVisible() | ||
| }) |
There was a problem hiding this comment.
Something like this had been deleted below (I think rightly, because whatever it is no longer has an ID in the same spot). The robot noticed it was the only place in the e2es where we were exercising this aspect of how IdRow works, so it wanted to add an assert about it back somewhere else where it would still work.
david-crespo
left a comment
david-crespo
left a comment
There was a problem hiding this comment.
Looks great!
Something for us to mull over: the instance cells here go to the instance page, but the source cells open a side modal. It's not a big deal, but it would be interesting if there was some way to indicate visually what to expect when you click. Maybe a link vs. button distinction, but it's hard to see how to do that here, plus buttons can open pages too.
| const deleteDisk = useApiMutation(api.diskDelete) | ||
| // no invalidation needed: the deleted snapshot is the transient one created | ||
| // by this flow, so nothing can be displaying it | ||
| const deleteSnapshot = useApiMutation(api.snapshotDelete) |
There was a problem hiding this comment.
There were some invalidations added here that were probably harmless but I'd rather avoid them because they give the impression of being necessary. On the other hand I'm always wondering #3083 whether we should be more aggressive about invalidations. But I don't think we tend to have issues.
2 participants
This PR adds a "source" cell to each row in the Disks table, and to each Instance's "Storage" tab.
For each of those locations, it provides a side modal with more info on the source disk / snapshot / image.
In the Disk sidemodal, the source cell is not a link, to prevent stacking of side modals. This follows the pattern on #3158. I also added a badge to the side modal to indicate whether the source is from an image or a snapshot. We can remove that if we feel like it's too noisy. I left it off on the main view, since you can click into it to get more details.
Note that this image detail side modal has a slightly different layout than the existing image detail side modal, and we might want to just use the existing one. I do think the existing one has more of a pseudo-form feel to it, which might be more appropriate in its current location as a drill-down on the images index list, which looks like this:
Closes #2057