-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Add opt-in modal behaviour to dialogs #5661
Add opt-in modal behaviour to dialogs #5661
Conversation
""" | ||
attr :id, :string, required: true | ||
attr :show, :boolean, default: false | ||
attr :modal, :boolean, default: false |
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.
I like the idea of conditionally disabling the phx-click-away
, but I feel the name of the attribute is a bit strange since the component is already called "modal".
Perhaps this was something you struggled with as well?
Brainstorming a bit:
- persistent
- sticky
- noBackdropDismiss
- preventBackdropClose
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.
Well, I wouldn't have called it a modal
of course :-)
Perhaps blocking
has a similar ring.
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.
confirmClose
maybe?
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.
These are separate concerns. Modal behaviour means that interacting with the main content is disabled until the user interacts with the dialog content. That means that clicking on the backdrop does nothing, not even asking for a confirmation.
This type of interaction is common for confirmation dialogs: they're not meant to be closed without clicking OK or Cancel.
In #5660 we agreed that asking for a confirmation provides a better UX for forms in a dialog. Nevertheless there will still be a need to be able to create actual modal dialogs.
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.
Shouldn't this also apply to pressing esc
?
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.
For accessibility, Escape to close should always be possible.
See the W3C Modal Dialog Example
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.
That's the one that trips me the most. So perhaps the best solution is to ask for confirmation indeed. Let's see what Chris has to say after the holidays. :)
I would instead suggest to add an option for the modal to confirm on any non-OK close action (clicking away, hitting 'Esc', pressing cancel etc). |
The core components is a starting point, you are welcome to add and remove any functionality that you want once it is in your app. You don't have to stay with Phoenix recommendations here. :) |
Actually, I have read the original issue and I understand it better. I am in favor of this but I would like a second pair of eyes. Apologies. |
@ArthurClemens I'm sorry that we kept this open so long that now, since we simplified the live generators a while back and removed the modal component, it's become obsolete... Therefore, I'm closing this. I do want to thank you very much for the contribution. If anyone comes across this and looks for a solution in their app, they can definitely use this as an example :) |
This PR resolves issue #5660
Changes:
modal
toCoreComponents
'modal
component, which if true prevents the dialog from closing when clicking outside.modal
attribute.