-
Notifications
You must be signed in to change notification settings - Fork 43
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
✨ Application tables: Refactor useEntityModal #1235
✨ Application tables: Refactor useEntityModal #1235
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #1235 +/- ##
==========================================
- Coverage 44.03% 44.02% -0.02%
==========================================
Files 177 177
Lines 4524 4516 -8
Branches 1009 1037 +28
==========================================
- Hits 1992 1988 -4
+ Misses 2521 2453 -68
- Partials 11 75 +64
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
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.
Small change requested for old forEach / array.push style we are stylistically moving away from.
let ids: number[] = []; | ||
applicationsToDelete.forEach((application) => { | ||
if (application.id) ids.push(application.id); | ||
}); | ||
if (ids) bulkDeleteApplication({ ids: ids }); | ||
setApplicationsToDelete([]); |
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.
let ids: number[] = []; | |
applicationsToDelete.forEach((application) => { | |
if (application.id) ids.push(application.id); | |
}); | |
if (ids) bulkDeleteApplication({ ids: ids }); | |
setApplicationsToDelete([]); | |
const ids = applicationsToDelete | |
.filter(application=>application?.id) | |
.map(application => application.id) | |
//TODO should have some sort of loading state / error state before closing the modal just incase the bulk delete fails. Then we could move the setApplicatoinsToDelete to a success/error callback after the async operation rather than it being synchronous as it sits here. | |
if (ids) bulkDeleteApplication({ ids: ids }); | |
setApplicationsToDelete([]); |
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.
@ibolton336, that's a good opportunity indeed.
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.
@ibolton336, good idea to move setApplicationsToDelete to the success/error callbacks.
We don't really need to handle loading or error state when deleting items, the notification whether the delete operation is successful or fails, by providing information to the user.
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Signed-off-by: Gilles Dubreuil <gdubreui@redhat.com>
Replace useEntityModal with in-file state.
See #1234 for details
Also: