Skip to content
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

Merged
merged 5 commits into from
May 31, 2022
Merged

feat: Delete workspace #1822

merged 5 commits into from
May 31, 2022

Conversation

presleyp
Copy link
Contributor

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.

@presleyp presleyp requested a review from a team as a code owner May 26, 2022 23:03
},
onError: {
target: "idle",
actions: ["assignBuildError", "displayBuildError"],
Copy link
Member

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?

Copy link
Contributor Author

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 :/

@Kira-Pilot
Copy link
Member

I realize this is outside of the scope of this PR. I was a bit surprised to see such a red dialog. I wonder if we could communicate the gravity of the situation with slightly less red, or perhaps some more contrast.
Definitely something we can discuss later!
Screen Shot 2022-05-27 at 9 20 24 AM

Copy link
Member

@Kira-Pilot Kira-Pilot left a 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.

@greyscaled
Copy link
Contributor

greyscaled commented May 27, 2022

I agree @Kira-Pilot ---> When dark theme was flipped on and our theme overrides removed, this is what resulted. I do think we should clean this up.

I will make a ticket for it.

@greyscaled
Copy link
Contributor

Added #1832

title={Language.deleteDialogTitle}
onConfirm={handleConfirm}
onClose={handleCancel}
description={<>{Language.deleteDialogMessage}</>}
Copy link
Contributor

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?

Comment on lines +44 to +46
const canDelete = (workspaceStatus: WorkspaceStatus) =>
["started", "stopped", "canceled", "error"].includes(workspaceStatus)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Praise: Nice 😎

Comment on lines +52 to +55
handleConfirm={() => {
workspaceSend("DELETE")
navigate("/workspaces")
}}
Copy link
Contributor

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

onSubmit={(values) => {
scheduleSend({
type: "SUBMIT_SCHEDULE",
autoStart: formValuesToAutoStartRequest(values),
ttl: formValuesToTTLRequest(values),
})
}}
/>
)
} else if (scheduleState.matches("submitSuccess")) {
navigate(`/workspaces/${workspaceId}`)
return <FullScreenLoader />
} else {

Copy link
Contributor Author

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

@presleyp presleyp merged commit 6f7b7f0 into main May 31, 2022
@presleyp presleyp deleted the delete-workspace/presleyp/768 branch May 31, 2022 14:43
kylecarbs pushed a commit that referenced this pull request Jun 10, 2022
* Add delete button

* Add confirmation dialog

* Extract dialog, storybook it, and test it

* Fix cancel and redirect

* Remove fragment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Frontend Workspace Delete
3 participants