-
-
Notifications
You must be signed in to change notification settings - Fork 735
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: update description on project deletion #7539
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
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.
Thanks for the clear PR description 🙌🏼
I like the idea of putting deletion information in the dialog too, but I'm not convinced about removing the deletion prerequisites when you don't need to do anything (refer to my inline comment). I'd like to hear other people's views.
Regarding your discussion point:
Should reminder to archive flags be reverted closer to disabled delete button, instead of placing it on the top.
I'm not sure what you mean. Could you pop in an image or something to clarify?
frontend/src/component/project/Project/ProjectSettings/Settings/DeleteProject.tsx
Outdated
Show resolved
Hide resolved
frontend/src/component/project/Project/DeleteProject/DeleteProjectDialogue.tsx
Outdated
Show resolved
Hide resolved
<strong>will permanently remove</strong> | ||
<ul> | ||
<li>all archived feature flags in this project</li> | ||
<li>API keys configured to access only this project</li> |
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.
Depending on how the API key deletion works, we might want to be a little more explicit (here or in the docs), but I don't have a clear suggestion for that yet. Let's see how the rest of this shakes out first.
Nice one 😄 |
</p> | ||
} | ||
/> | ||
</> |
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.
I think this can be a bit weird in some edge cases:
- featureCount === 0
- actionsCount > 0
- automatedActionsEnabled === false
We would only show "0 feature flags active", which I'm not sure is the intended behavior. I don't think it matters much, but I would pick one of:
- Show zeroes: No longer care about the total
featureCount
andactionsCount
totals, show them even if they are 0. This means we simply remove theConditionallyRender
withfeatureCount > 0 || actionsCount > 0
. - Don't show zeroes: Add a
ConditionallyRender
wrapper around each paragraph where we check the individual count for each along with any other conditions:featureCount > 0
actionsCount > 0 && isEnterprise() && automatedActionsEnabled
131c8c9
to
5f904e1
Compare
About the changes
Improving the project deletion experience by adding information about API keys and refining the messaging, making it easier to comprehend.
Improved Project Settings Deletion Section
Updated Delete Project Dialogue
Now reiterates that deleting a project will also remove all actions configured for the project, in addition to the project itself, its feature flags, and API keys.
Discussion points
Should reminder to archive flags be reverted closer to disabled delete button, instead of placing it on the top.
Prerequisits
#7503