-
Notifications
You must be signed in to change notification settings - Fork 15
feat: Add 'Delete Catalog' feature in the catalog details page #47
Conversation
jamieknight-db
left a comment
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! Thank you, Robert!
| { | ||
| onError: (error: Error) => { | ||
| setNotification(error.message, 'error'); | ||
| }, | ||
| } |
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.
This is nice 👍 we should do the same error handling on all the delete assets.
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.
Thank you @jamieknight-db! 😄
| Are you sure you want to delete the catalog | ||
| </Typography.Text> | ||
| <Typography.Text strong> | ||
| {` ${catalog}`} |
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.
nit: you can use {' '} for cleaner spacing
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 tip @Romanize!
yc-shawn
left a comment
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.
Nice!
|
Could someone squash and merge this PR? I do not have permissions. |
Closes #37
UC-DeleteCatalog.mov
Thanks to @jamieknight-db for providing a nice baseline with the delete table PR. 😄