Skip to content
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

Fix UpdateButton with custom notification doesn't close the confirmation dialog #9196

Merged
merged 1 commit into from
Aug 18, 2023

Conversation

fzaninotto
Copy link
Member

Closes #9194

@fzaninotto fzaninotto added the RFR Ready For Review label Aug 18, 2023
@fzaninotto fzaninotto changed the title Fix UpdateMutton with custom notification doesn't close the confirmation dialog Fix UpdateButton with custom notification doesn't close the confirmation dialog Aug 18, 2023
@djhi djhi added this to the 4.13.1 milestone Aug 18, 2023
@djhi djhi merged commit 9c63204 into master Aug 18, 2023
@djhi djhi deleted the fix-updatebutton-mutationoptions branch August 18, 2023 08:17
@slax57
Copy link
Contributor

slax57 commented Aug 21, 2023

FTR I don't agree with this fix.
To me this is just moving the issue to another place.

The root issue is not being able to close the dialog from within a side-effect fn, like onSuccess. We should instead provide a callback for that as an extra argument, that can be called in user land.

With this fix, the problem is no longer there if we provide a custom onSuccess, but we will have the same issue if we override the onSettled.
Besides, this is also technically a BC as, from now on, the dialog will also close in case of an error, while it would have remained open in the previous version (which makes it easier to retry the update).

@fzaninotto
Copy link
Member Author

You're absolutely right about the BC, I didn't see it. I'll reopen the issue.

@slax57
Copy link
Contributor

slax57 commented Aug 21, 2023

You're absolutely right about the BC, I didn't see it. I'll reopen the issue.

My bad! Reviewing PR #9208 made me realize that setOpen(false) used to be both in onSuccess and onError, so there is no BC!

However the rest of my comment is still valid I believe. Wdyt? Shoud we keep the fix or work on a better one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFR Ready For Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UpdateButton Confirm Dialogue Close Cannot Be Triggered with a Custom onSuccess Mutation Option (no redirect)
3 participants