Skip to content

Add a remove project button on the project settings page #15316

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

Merged
merged 3 commits into from
Dec 13, 2022

Conversation

selfcontained
Copy link
Contributor

@selfcontained selfcontained commented Dec 12, 2022

Description

Adds a "Remove Project" button on the project settings page.

image

Clicking the button brings up the same modal that is shown from the Projects list page when using the context menu action for removing.
image

Followup

To help keep this PR small I'll open a followup one that updates the Projects list page use the new <RemoveProjectModal/> component, along with a few other improvements for that page.

Related Issue(s)

Fixes #14251

How to test

Preivew Env: https://bmh-deleted93c550a0b.preview.gitpod-dev.com/

  • Create a new Project
  • Navigate to the Settings page for that project
  • Scroll to the bottom and click the Remove Project button
  • You should see a confirmation modal that you can dismiss, or proceed with.
  • After the project is deleted you should get navigated to the Projects list page

Release Notes

Projects can now be deleted from the corresponding Settings page for that project.

Documentation

Werft options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-large-vm
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-bmh-delete-project-settings.2 because the annotations in the pull request description changed
(with .werft/ from main)

@selfcontained selfcontained changed the title wip for remove project on settings page Add a remove project button on the project settings page Dec 12, 2022
@@ -107,3 +107,7 @@ const FeatureFlagContextProvider: React.FC = ({ children }) => {
};

export { FeatureFlagContext, FeatureFlagContextProvider };

export const useFeatureFlags = () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes it just a bit simpler to access feature flags from a component.

@@ -61,33 +65,52 @@ export default function () {
}
}, [team]);

if (!project) return null;
const updateProjectSettings = useCallback(
Copy link
Contributor Author

@selfcontained selfcontained Dec 12, 2022

Choose a reason for hiding this comment

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

Wrapped each of these functions with useCallback() to avoid creating new references for each render. The guts of each function shouldn't have changed at all.

}, [history, team]);

// TODO: Render a generic error screen for when an entity isn't found
if (!project) return null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should live after all hooks, so bumped it down since I added a few useCallback() wrappers on the fns that weren't there before.

@selfcontained selfcontained marked this pull request as ready for review December 12, 2022 23:28
@selfcontained selfcontained requested a review from a team December 12, 2022 23:28
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Dec 12, 2022
@selfcontained selfcontained mentioned this pull request Dec 13, 2022
4 tasks
Copy link
Member

@svenefftinge svenefftinge left a comment

Choose a reason for hiding this comment

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

Thank you! TIL about useCallback 🙈
I wonder what the implications of removing a project are. We have other resources that reference projects such as workspaces and prebuilds. One could argue that those are even owned by a project, but deleting all this together with a project would seem like a quite impactful operation. So we probably don't want to do that, but then we should indicate to users when a workspace references a deleted project.
Also, do we need to make sure that prebuilds created for the removed project are no longer used when starting new workspaces?

/hold

@easyCZ
Copy link
Member

easyCZ commented Dec 13, 2022

I wonder what the implications of removing a project are. We have other resources that reference projects such as workspaces and prebuilds. One could argue that those are even owned by a project, but deleting all this together with a project would seem like a quite impactful operation.

The change, as presented in this PR only improves the UI aspect of project deletion. As such, the above feels not directly relevant to this change.

Were you referring to also deleting stored (cached/state) resources which relate to the project? Or were you referring to the workflow on what happens when you delete a project on the server-side?

Copy link
Member

@easyCZ easyCZ left a comment

Choose a reason for hiding this comment

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

Nice! Really like the cleanup you're doing alongside the improvements themselves. For me, this change does exactly what it should, and what was specified in the original issue.

LGTM

Feel free to land, if you feel the conversations have been resolved.

@svenefftinge
Copy link
Member

svenefftinge commented Dec 13, 2022

The change, as presented in this PR only improves the UI aspect of project deletion.

Indeed 🙏 I had somehow the impression it would introduce the removal of projects.

/unhold

@roboquat roboquat merged commit b9dab72 into main Dec 13, 2022
@roboquat roboquat deleted the bmh/delete-project-settings branch December 13, 2022 09:34
Copy link
Contributor

@gtsiolis gtsiolis left a comment

Choose a reason for hiding this comment

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

Hey @selfcontained!

Coming a bit late here but UX LGTM. 🏁

  1. Left some minor non-blocking comments below.
  2. Also, closed Delete projects from project settings #14251 in favor of Move project removal action under project settings #7273 which also contains some design specs that could be helpful here, see Move project removal action under project settings #7273 (comment).

@@ -237,6 +260,22 @@ export default function () {
</Alert>
)}
</div>
<div>
<h3 className="mt-12">Delete Project</h3>
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: What do you think of using a different verb here to avoid causing any confusion whether this is also going to delete a project or repository on the provider?

Suggested change
<h3 className="mt-12">Delete Project</h3>
<h3 className="mt-12">Remove Project</h3>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like it. I’ll include this change in a follow up pr

return (
<ConfirmationModal
title="Remove Project"
areYouSureText="Are you sure you want to remove this project from this team? Team members will also lose access to this project."
Copy link
Contributor

Choose a reason for hiding this comment

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

issue(non-blocking): This is out of the scope here, but there's a top margin here that's not needed in the modal component. Posting below how the modal would look like be ideally for visibility. However, feel free to leave this as is.

BEFORE AFTER
modal-before modal-after

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, I'll update the confirmation modal in the follow up too.

@roboquat roboquat added the deployed: webapp Meta team change is running in production label Dec 13, 2022
@roboquat roboquat added the deployed Change is completely running in production label Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note size/L team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delete projects from project settings
5 participants