-
Notifications
You must be signed in to change notification settings - Fork 6
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
Change link/unlink behaviour to directly apply changes without showing modal or message #2523
Conversation
…g modal or message
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.
The functionality is good but the refactored code looks like you did some refactoring but didn't finish the changes.
As I mentioned in my inline comments, we should do 1 of 2 things. Either:
- keep
deleteResponse
anddeleteError
the way they were and undo the change to calldeleteBatchAssociationTuples
in the functiondeleteResponse
would be changed to:setShowPureBinarySpinner(false); setUnlinkPureBinaryModalProps(null); updateRecordPage(true, LogReloadCauses.RELATED_BATCH_UNLINK);
- rename
deleteResponse
todeleteTuples
since it's now calling the delete function and handling both the "delete response" and "delete error"- remove
deleteError
as a named function and put its contents directly in thecatch
block here since the function is only used in 1 place
- remove
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.
Functionality is good, changes to test cases are good too. Only need to cleanup the changes based on my inline comments.
A good rule to follow is to review your own changes here on github in the browser so you can see how all your changes look compared to everything else.
Link/Unlink behavior updated to not show the confirmation/summary modals or message when the link/unlink is successful.
Resolves #2273