-
Notifications
You must be signed in to change notification settings - Fork 22
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
#9092: slice 2.3 support deleting user deployment in extension console #9286
#9092: slice 2.3 support deleting user deployment in extension console #9286
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9286 +/- ##
==========================================
+ Coverage 74.24% 75.16% +0.91%
==========================================
Files 1332 1368 +36
Lines 40817 42243 +1426
Branches 7634 7866 +232
==========================================
+ Hits 30306 31751 +1445
+ Misses 10511 10492 -19 ☔ View full report in Codecov by Sentry. |
Playwright test resultsDetails Open report ↗︎ Flaky testsmsedge-setup › setup/unaffiliated.setup.ts › authenticate with unaffiliated user Skipped testschrome › tests/runtime/googleSheetsIntegration.spec.ts › can activate a google spreadsheet mod with config options |
src/activation/modOptionsHelpers.ts
Outdated
* (no action is taken, and we return undefined) | ||
* TODO: Handle activating a mod that is updating to a newer version and update the personal deployment to point to the new mod version | ||
*/ | ||
export function useHandlePersonalDeploymentOption() { |
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.
Can we choose a more informative name than "handle"? E.g.:
useUpdateOrDeletePersonalDeployment
useCreateDeletePersonalDeployment
Potentially even calling out that it's to sync that state with the backend, e.g., useUpdateOrDeletePersonalDeploymentOnServer
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.
Since it will create, delete, fetch and in the future update personal deployments, I think a more succinct name like: useManagePersonalDeployment
works here.
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.
LGTM - see suggestions on naming, dead code, and typescript failures
When the PR is merged, the first loom link found on this PR will be posted to |
…-deleting-user-deployment-in-extension-console
What does this PR do?
Demo
Future Work
Checklist
Leave all that are relevant and check off as completed
For more information on our expectations for the PR process, see the
code review principles doc