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

bug: Modal property canDismiss complicated usage since v6.4.0 and interface missing #26544

Closed
4 of 7 tasks
Unkn0wn0x opened this issue Dec 29, 2022 · 5 comments
Closed
4 of 7 tasks
Labels
holiday triage issues that were created during holiday period

Comments

@Unkn0wn0x
Copy link

Unkn0wn0x commented Dec 29, 2022

Prerequisites

Ionic Framework Version

  • v4.x
  • v5.x
  • v6.x
  • Nightly

Current Behavior

Related feature request: #26292
Related pull request: #26384


It has been forgotten to update the <ion-modal> interface definition, as you can see here for the property canDismiss:

The interface definition for canDismiss does not allow the new data and role parameter, whereas the modal itself can handle it.

Same for the official documentation here:

On the other hand the usage of the modal canDismiss property is imho too complicated, f. e. with the deprecated swipeToClose attribute it looked like following:

const modal: HTMLIonModalElement = await this.modalController.create({
    component: MyPage,
    canDismiss: true,
    swipeToClose: false,
    presentingElement: await this.modalController.getTop()
});
await modal.present();

Now (with v.6.4.1) it looks like the following:

const modal: HTMLIonModalElement = await this.modalController.create({
    component: MyPage,
    canDismiss: async(data, role) => role !== 'gesture' ? true : false,
    presentingElement: await this.modalController.getTop()
});
await modal.present();

This works as expected, if I call the method this.modalCtrl.dismiss() the modal closes, if I try to swipe-to-close it, it does not work. It's also possible to pass arguments within this.modalCtrl.dismiss(data, role).

But why it requires an async function with the role parameter as second argument? Also there is a lack of documentation for the possible (default) role's, f. e. gesture when trying to swipe-to-close the modal.

Expected Behavior

Updated usage documentation:

Updated interface:

Maybe a simpler usage of the canDismiss property as described in #26292 with the role argument at first place?

Steps to Reproduce

Try to pass a function instead of an boolean on the canDismiss property, the compiler will throw an error, that's not possible.

Code Reproduction URL

No response

Ionic Info

Ionic:

   Ionic CLI                     : 6.20.6 (/opt/homebrew/lib/node_modules/@ionic/cli)
   Ionic Framework               : @ionic/angular 6.4.1
   @angular-devkit/build-angular : 15.0.4
   @angular-devkit/schematics    : 15.0.4
   @angular/cli                  : 15.0.4
   @ionic/angular-toolkit        : 7.0.0

Capacitor:

   Capacitor CLI      : 4.6.1
   @capacitor/android : not installed
   @capacitor/core    : 4.6.1
   @capacitor/ios     : 4.6.1

Utility:

   cordova-res : 0.15.4
   native-run  : 1.7.1

System:

   NodeJS : v18.11.0 (/opt/homebrew/Cellar/node/18.11.0/bin/node)
   npm    : 8.19.2
   OS     : macOS

Additional Information

No response

@ionitron-bot ionitron-bot bot added the holiday triage issues that were created during holiday period label Dec 29, 2022
@ionitron-bot
Copy link

ionitron-bot bot commented Dec 29, 2022

Thanks for the issue! This issue has been labeled as holiday triage. With the winter holidays quickly approaching, much of the Ionic Team will soon be taking time off. During this time, issue triaging and PR review will be delayed until the team begins to return. After this period, we will work to ensure that all new issues are properly triaged and that new PRs are reviewed. In the meantime, please read our Winter Holiday Triage Guide for information on how to ensure that your issue is triaged correctly. Thank you!

@sean-perkins
Copy link
Contributor

Hello @Unkn0wn0x appreciate you reporting this issue!

I've opened two PRs to fix the documentation and type definition for the ModalOptions interface.

The majority of the team is on vacation for the holidays until after the New Year. I'm going to leave this issue untriaged in the interim, so that the team can discuss your feedback regarding swipeToClose vs. the canDismiss implementation pattern.

@Unkn0wn0x
Copy link
Author

@sean-perkins Thanks for your efforts, looks and sounds good to me.

And I wish you already a happy new year!

@sean-perkins
Copy link
Contributor

Hello @Unkn0wn0x I spoke with the team about the state of canDismiss.

We reached the conclusion that the migration from swipeToClose to canDismiss is still a documentation issue. I've created a backlog task to add new component playground examples (after the fix is shipped) to demonstrate how developers can prevent swiping to close or the hardware back button.

The reason for this decision, is the current swipeToClose property and behavior is imperfect. The intention is to prevent users from escaping/dismissing the modal outside of interaction within the modal. swipeToClose only prevents swiping. This works for iOS, but misses the fact that Android users can use the hardware back button and that will also dismiss the modal. Knowing this, we think that swipeToClose is leading to improper implementations and not guiding developers to the best scenario for their app.

As with all things, decisions are fluid and as we learn more and gauge the community migrating implementations in v7, we will continue to try and improve the developer experience, if gaps exist.

@ionitron-bot
Copy link

ionitron-bot bot commented Feb 3, 2023

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Feb 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
holiday triage issues that were created during holiday period
Projects
None yet
Development

No branches or pull requests

2 participants