fix(site): revamp UI for batch-updating workspaces #18895
Conversation
| isLoading={batchActions.isProcessing} | ||
| onClose={() => setActiveBatchAction(undefined)} | ||
| onConfirm={async () => { | ||
| workspacesToUpdate={checkedWorkspaces} |
There was a problem hiding this comment.
Renamed prop because I don't think that a component should need to be aware of how its parent chooses to represent workspace selections
|
@aslilac I don't think there's a good way to ping you specifically now that you're a code owner, but I was hoping I could have you review this PR |
| args: { | ||
| open: true, | ||
| isProcessing: false, | ||
| onSubmit: () => window.alert("Hooray! Everything has been submitted"), |
There was a problem hiding this comment.
I believe Storybook exports a action function that you can call which shows a more subtle notification that a specific thing happened. iirc the signature is (string) => void so you can still pass a description.
| import { BatchUpdateModalForm } from "./BatchUpdateModalForm"; | ||
| import { ACTIVE_BUILD_STATUSES } from "./WorkspacesPage"; | ||
|
|
||
| type Writeable<T> = { -readonly [Key in keyof T]: T[Key] }; |
There was a problem hiding this comment.
I was so prepared to tell you to use a built-in TypeScript util but apparently somehow we don't have a standard inverse of Readonly 🤦♀️ is this a useful enough thing to justify having in a different file for reuse?
| }; | ||
|
|
||
| export const OnlyReadyToUpdate: Story = { | ||
| beforeEach: (ctx) => { |
There was a problem hiding this comment.
| beforeEach: (ctx) => { | |
| beforeEach: (story) => { |
I think this makes it a bit clearer what object we're dealing with (it appears to be the fully normalized story)
| }; | ||
|
|
||
| export const NoWorkspacesToUpdate: Story = { | ||
| beforeEach: (ctx) => { |
| ctx.parameters = { ...ctx.parameters, queries }; | ||
| }, | ||
| decorators: [ | ||
| (Story, ctx) => { |
There was a problem hiding this comment.
although taking (Story, story) as arguments would be, uh, bad. hmm.
| {/* | ||
| * Because of how the Dialog component works, we need to make | ||
| * sure that at least the parent stays mounted at all times. But | ||
| * if we move all the state into ReviewForm, that means that its | ||
| * state only mounts when the user actually opens up the batch | ||
| * update form. That saves us from mounting a bunch of extra | ||
| * state and firing extra queries, when realistically, the form | ||
| * will stay closed 99% of the time while the user is on the | ||
| * workspaces page. | ||
| */} |
There was a problem hiding this comment.
you don't actually explain what it is about the Dialog component that requires this tho. I think the benefits of not mounting a component when you don't need to should be relatively obvious, so this very wordy comment really just raises more questions than answer for me.
| onSubmit: () => void; | ||
| }>; | ||
|
|
||
| export const BatchUpdateModalForm: FC<BatchUpdateModalFormProps> = ({ |
There was a problem hiding this comment.
I feel weird about the titular component being the last thing in the file
| <li | ||
| key={ws.id} | ||
| className="[&:not(:last-child)]:border-b-border [&:not(:last-child)]:border-b [&:not(:last-child)]:border-solid border-0" | ||
| > |
There was a problem hiding this comment.
This ReviewPanel component is always rendered inside of this exact li, long className and all. could this be refactored?
| id={failedValidationId} | ||
| className="m-0 text-highlight-red text-right text-sm pt-2" | ||
| > | ||
| Please acknowledge consequences to continue. |
There was a problem hiding this comment.
tbh this feels a little overkill. restarting a workspace can be destructive, but in practice it shouldn't be.
| workspaces: readonly Workspace[]; | ||
| queries: QueryEntry; | ||
| }>; | ||
| function createPatchedDependencies(size: number): PatchedDependencies { |
There was a problem hiding this comment.
the more I look at these stories the more I'm worried that this function is the wrong abstraction. we're doing all kinds of hoop jumping to make it work rn and I wonder if it'd just be easier and clearer to write out the workspaces and query results like we usually do.
|
also sorry I didn't review this sooner. I have no idea how this slid under my radar, I did a ton of reviews on thursday and just absolutely missed this one. |
Closes #18879
Builds on #18895
Changes made
BatchUpdateConfirmationcomponent, replacing it withBatchUpdateModalFormScreenshots
Notes