-
Notifications
You must be signed in to change notification settings - Fork 23
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
Make modal backdrop dismiss configurable #3191
Make modal backdrop dismiss configurable #3191
Conversation
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.
Thanks for your contribution and good suggestion!
I've added a couple of suggestions that will most likely have consequences for the tests, so I will await your responses before looking at the spec.
{ | ||
name: 'backdropDismiss', | ||
description: `(Optional) Modal will close on backdrop click`, | ||
defaultValue: 'undefined', |
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.
Even if not technically true, I would describe the default value as true here. This is what Ionics default value is on the modal, and thus how our modal behaves out of the box.
@@ -24,6 +24,7 @@ export interface ModalConfig { | |||
// drawer properties | |||
interactWithBackground?: boolean; | |||
cssClass?: string | string[]; | |||
backdropDismiss?: boolean; |
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.
Consider moving this to line 22 for readability
@@ -74,6 +74,7 @@ export class ModalExampleSimpleComponent implements OnInit { | |||
showNestedOptions: true, | |||
showDummyContent: false, | |||
showModalSizeSelector: true, | |||
backdropDismiss: 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.
return config.backdropDismiss; | ||
} | ||
|
||
return config.flavor === 'compact' || config.interactWithBackground ? false : true; |
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 think this should be refactored so these are the first checks. We do not want people to be able to overwrite backdropDismiss
for compact or interactWithBackground
scenarios :)
Together with @Tobias-Secher and team it has been determined (from a usability standpoint) that we do not wish to support this feature, and I know that they ended up with a better solution by using the |
Which issue does this PR close?
This PR closes #3162
What is the new behavior?
Does this PR introduce a breaking change?
Are there any additional context?
Checklist:
The following tasks should be carried out in sequence in order to follow the process of contributing correctly.
Reminders
Review
When the pull request has been approved it will be merged to
develop
by Team Kirby.