-
Notifications
You must be signed in to change notification settings - Fork 2
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
Austenem/CAT-386 add confirmation window #3479
Conversation
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.
Great work! Minor comments.
...tic/js/components/workspaces/ConfirmDeleteWorkspacesDialog/ConfirmDeleteWorkspacesDialog.tsx
Outdated
Show resolved
Hide resolved
@@ -176,3 +176,11 @@ export function filterObjectByKeys<O extends object, K extends keyof O>(obj: O, | |||
export function getOriginSamplesOrgan(entity: { origin_samples_unique_mapped_organs: string[] }) { | |||
return entity.origin_samples_unique_mapped_organs.join(', '); | |||
} | |||
|
|||
export function generateCommaList(list: string[]): string { |
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 this is meant for re-use, it would be nice to add a spec or two.
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 used JSDoc - if there's another preferred documentation format though, let me know!
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 was only asking for a test or two, but this is extra helpful!
console.error(e); | ||
}); | ||
|
||
selectedWorkspaceIds.clear(); |
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 we want to only clear the selected ids if the delete was successful? Should we also add a toastSuccess
?
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.
That's a good point! Adding a success indicator is also a good idea.
...tic/js/components/workspaces/ConfirmDeleteWorkspacesDialog/ConfirmDeleteWorkspacesDialog.tsx
Outdated
Show resolved
Hide resolved
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.
Great work! I did spot some minor revisions to imports (I don't think we've had a chance to go over this in the past), and noticed an opportunity to add a test case. Let me know if I can help or clarify!
import { | ||
Box, | ||
Button, | ||
Dialog, | ||
DialogActions, | ||
DialogContent, | ||
DialogTitle, | ||
Divider, | ||
IconButton, | ||
Stack, | ||
} from '@mui/material'; |
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.
Unfortunately, since the site builds as a CommonJS module, it's best to import these separately, e.g.:
import Box from '@mui/material/Box';
import Button from '@mui/material/Button';
...etc...
Basically - importing directly from @mui/material
indirectly pulls in the entire @mui/material
package/everything in it since webpack with CommonJS modules can't treeshake unused code.
Further reading:
- https://webpack.js.org/guides/tree-shaking/
- https://mui.com/material-ui/guides/minimizing-bundle-size/#when-and-how-to-use-tree-shaking
- We have a task to migrate to ESM so that treeshaking works as expected: https://hms-dbmi.atlassian.net/browse/CAT-124
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.
Ahh gotcha! That's good to know.
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 quick changes. One minor comment.
@@ -176,3 +176,11 @@ export function filterObjectByKeys<O extends object, K extends keyof O>(obj: O, | |||
export function getOriginSamplesOrgan(entity: { origin_samples_unique_mapped_organs: string[] }) { | |||
return entity.origin_samples_unique_mapped_organs.join(', '); | |||
} | |||
|
|||
export function generateCommaList(list: string[]): string { |
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 was only asking for a test or two, but this is extra helpful!
...tic/js/components/workspaces/ConfirmDeleteWorkspacesDialog/ConfirmDeleteWorkspacesDialog.tsx
Outdated
Show resolved
Hide resolved
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.
Looks good to me!
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.
Great job on the fixes!
Summary
This update adds a confirmation modal to the 'Delete Workspace' button to avoid accidental deletions.
Design Documentation/Original Tickets
CAT-386 JIRA ticket (dev)
CAT-780 JIRA ticket (design)
Figma mockup
Testing
Functionalities of this feature that have been tested manually:
Screenshots/Video
Screen.Recording.2024-07-24.at.11.00.05.AM.mov
Checklist
CHANGELOG-your-feature-name-here.md
is present in the root directory, describing the change(s) in full sentences.