-
Notifications
You must be signed in to change notification settings - Fork 889
feat: Delete workspace #1822
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
feat: Delete workspace #1822
Conversation
}, | ||
onError: { | ||
target: "idle", | ||
actions: ["assignBuildError", "displayBuildError"], |
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.
Do these show an error toast or something similar?
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.
Yes, and now that you mention it I should test for it. The component is called Snackbar. Oh but I think redirecting will stop it from happening :/
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.
Works great!!!! Nice job.
I will make a ticket for it. |
Added #1832 |
title={Language.deleteDialogTitle} | ||
onConfirm={handleConfirm} | ||
onClose={handleCancel} | ||
description={<>{Language.deleteDialogMessage}</>} |
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.
Nit: Are we able to pass the string directly without the fragment?
const canDelete = (workspaceStatus: WorkspaceStatus) => | ||
["started", "stopped", "canceled", "error"].includes(workspaceStatus) | ||
|
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.
Praise: Nice 😎
handleConfirm={() => { | ||
workspaceSend("DELETE") | ||
navigate("/workspaces") | ||
}} |
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.
Question:
What happens if we send "DELETE"
but the API request fails? It seems we would navigate away.
I ran into this problem in #1701 and used a submitSuccess
state to indicate it was OK to navigate.
WDYT here?
Edit: LoC for reference
coder/site/src/pages/WorkspaceSchedulePage/WorkspaceSchedulePage.tsx
Lines 169 to 181 in ebaae75
onSubmit={(values) => { | |
scheduleSend({ | |
type: "SUBMIT_SCHEDULE", | |
autoStart: formValuesToAutoStartRequest(values), | |
ttl: formValuesToTTLRequest(values), | |
}) | |
}} | |
/> | |
) | |
} else if (scheduleState.matches("submitSuccess")) { | |
navigate(`/workspaces/${workspaceId}`) | |
return <FullScreenLoader /> | |
} else { |
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.
You're right. I opened a decision ticket on this and we decided to stick with what I have for now, but change it in the future. #1837
* Add delete button * Add confirmation dialog * Extract dialog, storybook it, and test it * Fix cancel and redirect * Remove fragment
Closes #768
I broke out the confirmation dialog so it can be in storybook.
I have it redirect to the workspaces page when you confirm deletion. The workspaces page doesn't poll I guess so you won't see the change happen there, but we can change that later. When deletion finishes, the workspace page doesn't get a workspace object with the property of being deleted, but an error, so redirection seemed like the way to go.