-
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
onCancel event #6049
Comments
I think it would need to be added to the source code too. |
I wanted to do it, but I don't really know in which category we shoul put the |
It goes into the UI category if it has the |
Here is a JSFiddle with a dialog element and the |
FYI, it's just an |
Tried implementing support for this here: Please let me know it i've made any mistakes :) |
Should React support for the onClose event too? It's also related to the dialog. @ludvigsen you were faster than me, haha, grats =]. I've created an example page (took examples/basic/index.html as source) to test out new events on gist, you can check it out and use it for easier debugging. |
Thanks @cbrwizard! It seems like it would make a lot of sense to support the onClose event as well, I can definitely add it. Although in react I guess you would control the dialog through the "open" attribute, and therefore know when the user closed the dialog. Whereas the cancel event is fired if the user presses ESC, so the only way to know about it is to listen for it. |
Not really, because you can open a dialog as a normal dialog, or as a modal. Afaik, there's no way with the attribute to open it as a modal. |
@tleunen thats true, would be nice if it was possible to control modal as well with an attribute. |
Is there still a issue here, or should this be closed? |
@aweary thanks. I'll look into that PR and see if I can help |
I see that #6247 was merged, should this issue be closed? |
It should, thank you! |
I don't see the
onCancel
event in the list of supported events. Would it be possible to add it in the list?It's used by the Dialog element, already available in Chrome.
The text was updated successfully, but these errors were encountered: