-
Notifications
You must be signed in to change notification settings - Fork 47.6k
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
6049 add support for onCancel event #6247
Conversation
8d16f33
to
dca391b
Compare
@ludvigsen updated the pull request. |
d3b8530
to
654b318
Compare
@ludvigsen updated the pull request. |
This might not be needed at all, but I think that this change will benefit from tests. Like in this file. |
@cbrwizard I don't think jsdom supports the Dialog element, so testing this could prove an issue, at least through jsdom/jest. |
@ludvigsen ping |
@ludvigsen I took your code, rebased and fixed conflicts. I don't know if it helps but I'd at least like to give this issue a bit of visibility. |
@devjunkORG Thanks, I appreciate it. Did you check to see if stuff still works as well? |
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.
@ludvigsen looks like there are a number of conflicts. Would you be able to resolve those? I'd like to make sure this gets in.
@aweary Great! I'll try to resolve the conflicts. |
654b318
to
4cbc434
Compare
4cbc434
to
318563d
Compare
@aweary Resolved the conflicts also added onClose event for completeness |
Thanks @ludvigsen! I verified locally that the events are being dispatched for |
Adds support for the onCancel event described here: #6049