-
Notifications
You must be signed in to change notification settings - Fork 320
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
Soft delete of jobs and datasets. Style dialog component #2343
Conversation
Signed-off-by: tito12 <[email protected]>
A much needed addition to the UI! Looks great, we should also added deletion of namespaces as a follow up feature. I'll let @phixMe provide a more thorough review. |
web/src/components/Dialog.tsx
Outdated
@@ -26,16 +27,28 @@ export default function AlertDialog(props: IProps) { | |||
<div> |
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 need to have this encapsulating tag?
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 component exist previously with encapsulating tag. Moved it to functional component as rest part components.
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
}, | ||
buttonDelete: { | ||
backgroundColor: theme.palette.error.main, | ||
color: 'white', |
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 I like doing the theme.palette.common.white
for these in case we ever wish to support light mode.
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.
Sure, added it
> | ||
{i18next.t('jobs.delete')} | ||
</Button> | ||
<Dialog |
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.
There are two instances of this dialog... Can the dialog itself just be a reusable component.
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 component is reusable and located in "components/Dialog.tsx"
, and use it with different props
<Box display={'flex'} alignItems={'center'}> | ||
<Box mr={1}> | ||
<Button | ||
color='primary' |
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 make this a ghost
variant... I think it's a little too strong on the page.
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.
Sure, good point
web/src/components/Dialog.tsx
Outdated
onClick={props.ignoreWarning} | ||
> | ||
Continue | ||
</Button> |
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 make these small buttons inside the dialog?
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.
Sorry, can you explain what do you mean by that?
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 we could possibly use smaller buttons here since the dialog is so minute, but maybe it's not necessary.
Signed-off-by: tito12 <[email protected]>
Signed-off-by: tito12 <[email protected]>
Signed-off-by: tito12 <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #2343 +/- ##
============================================
+ Coverage 67.15% 76.72% +9.56%
- Complexity 231 1177 +946
============================================
Files 40 222 +182
Lines 947 5354 +4407
Branches 101 429 +328
============================================
+ Hits 636 4108 +3472
- Misses 163 768 +605
- Partials 148 478 +330
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Signed-off-by: tito12 <[email protected]>
Signed-off-by: tito12 <[email protected]>
Signed-off-by: tito12 <[email protected]>
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, looks good!
Signed-off-by: tito12 [email protected]
Problem
According to the issue #2167 we need option to soft delete records, it's already possible from API but not from UI.
Solution
Added option for jobs and datasets to be able to soft delete record with dialog component and double confirmation from UI side.
Checklist
CHANGELOG.md
with details about your change under the "Unreleased" section (if relevant, depending on the change, this may not be necessary).sql
database schema migration according to Flyway's naming convention (if relevant)