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

feat: ios card style modal for iPad should not affect routerOutlet #20700

Closed
DwieDima opened this issue Mar 5, 2020 · 15 comments
Closed

feat: ios card style modal for iPad should not affect routerOutlet #20700

DwieDima opened this issue Mar 5, 2020 · 15 comments
Labels
package: core @ionic/core package type: bug a confirmed bug report
Milestone

Comments

@DwieDima
Copy link
Contributor

DwieDima commented Mar 5, 2020

Feature Request

Ionic version:

[x] 5.0.4

Describe the Feature Request

At the moment the card-style-modal only looks good on mobile view. On tablet view and especially with split-pane there is a lot of potential for improvement.
There should be an option to open a modal as usual. Subsequent nested modals should not have any influence on the routerOutlet, but only on the underlying modal

Current behaviour:

Link to video

Describe Preferred Solution

calling a child modal by using

presentingElement: await this.modalController.getTop()

should appear as in the picture by providing additional property like

  public async onOpenModal() {
    const modal = await this.modalController.create({
      component: TestPage,
      swipeToClose: true,
      presentingElement: await this.modalController.getTop(),
      style: 'tablet'
    });
    return await modal.present();
  }

furthermore nested modals should respect the given width/height provided by cssClass.

Additional Context

You can find this kind of card style modal in the IOS Appstore

@ionitron-bot ionitron-bot bot added the triage label Mar 5, 2020
@liamdebeasi
Copy link
Member

Thanks for the issue. In the case of a split view application, you should be targeting ion-split-view, not ion-router-outlet since that will not account for the side menu. Can you try targeting that instead and see if the issue persists?

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Mar 5, 2020
@ionitron-bot ionitron-bot bot removed the triage label Mar 5, 2020
@DwieDima
Copy link
Contributor Author

DwieDima commented Mar 5, 2020

Thanks for the issue. In the case of a split view application, you should be targeting ion-split-view, not ion-router-outlet since that will not account for the side menu. Can you try targeting that instead and see if the issue persists?

Could you provide a snipped on how to target ion-split-view instead of ion-router-outlet for creating a card style modal?
I cannot find any IonSplitView, but only IonSplitPane.
I can't find the property nativeEl on IonSplitPane.

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Mar 5, 2020
@liamdebeasi
Copy link
Member

Sorry, I mean ion-split-pane. You can just use a document.querySelector or use Angular's ViewChild to select it.

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Mar 5, 2020
@ionitron-bot ionitron-bot bot removed the triage label Mar 5, 2020
@DwieDima
Copy link
Contributor Author

DwieDima commented Mar 6, 2020

Sorry, I mean ion-split-pane. You can just use a document.querySelector or use Angular's ViewChild to select it.

Yes, now also the ion-split-pane get affected. Thank you.
Following code was used:

  public async onOpenModal() {
    const modal = await this.modalController.create({
      component: TestPage,
      swipeToClose: true,
      presentingElement: document.querySelector('ion-split-pane')
    });
    return await modal.present();
  }

But this was not the intention of my feature request.
Sorry for misleading feature title, i'll try to explain by pictures:

Current behaviour using card style modal:

Wanted behaviour using card style modal:

as you can see, the second level modal is not taking the full width, but respects width and height of parent modal.

for comparison, this is what you get if you:

  1. create regular modal (without presentingElement)
  2. inside that modal call another modal using modalController.getTop() as presentingElement

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Mar 6, 2020
@liamdebeasi
Copy link
Member

Thanks for the clarification! I am going to change this to a bug since Ionic's card style modal does not follow the spec for iPad, but it should.

@liamdebeasi liamdebeasi added package: core @ionic/core package type: bug a confirmed bug report labels Mar 6, 2020
@ionitron-bot ionitron-bot bot removed the triage label Mar 6, 2020
@liamdebeasi liamdebeasi added this to the 5.0.6 milestone Mar 11, 2020
@DwieDima
Copy link
Contributor Author

@liamdebeasi i tried your dev build dev.202003111951.2b4c675.

It works just like I thought it would!
But I have discovered a few little things:

  • property backdropDismiss: true is not working
  • property cssClass: 'default-modal-size' wont apply height and width
    • by default the modal wont take size of normal modal (600x600 px)

default-modal class in global.scss:

.default-modal-size {
    --width: 600px;
    --height: 600px;
}

used code for parent modal:

  public async onOpenModal() {
    const modal = await this.modalController.create({
      component: SettingsPage,
      cssClass: 'default-modal',
      backdropDismiss: true,
      swipeToClose: true,
      presentingElement: this.routerOutlet.nativeEl
    });
    return await modal.present();
  }

used code for child modal:

  • side question: assumed cssClass would work on the parent modal. Will the child modal automatically take its size?
  public async openModal() {
    const modal = await this.modalController.create({
      component: SettingsPage,
      backdropDismiss: true,
      swipeToClose: true,
      presentingElement: await this.modalController.getTop()
    });
    return await modal.present();
  }

Furthermore i think, there should still be the option to call fullscreen modals. For example, you can find these in the mail app. Heres a screenshot:

also heres a link to a video of current behavior of your dev build:
For comparison you can see the normal modal too.
Link to video

@liamdebeasi
Copy link
Member

liamdebeasi commented Mar 12, 2020

Thanks for the useful feedback!

  1. The backdrop dismiss is intentional. There does not seems to be a backdrop dismiss in a native modal on iPadOS when I tested in a native app. (I will double check on this, but I am pretty sure it is supposed to be this way)

  2. The card modal sets its own width/height via CSS variables, so I will need to make sure users can overrides these.

  3. The child modal will not taken on the size of the parent modal -- they are two separate instances. This is consistent with how other modals work in Ionic.

  4. We can investigate doing a fullscreen modal, but this seems to be separate from the card style modal so it might be best to create a separate feature request.

I will work on these changes and will post another dev build for testing. Thank you!

@DwieDima
Copy link
Contributor Author

Thanks for the useful feedback!

  1. The backdrop dismiss is intentional. There does not seems to be a backdrop dismiss in a native modal on iPadOS when I tested in a native app. (I will double check on this, but I am pretty sure it is supposed to be this way)
  2. The card modal sets its own width/height via CSS variables, so I will need to make sure users can overrides these.
  3. The child modal will not taken on the size of the parent modal -- they are two separate instances. This is consistent with how other modals work in Ionic.
  4. We can investigate doing a fullscreen modal, but this seems to be separate from the card style modal so it might be best to create a separate feature request.

I will work on these changes and will post another dev build for testing. Thank you!

to your point 4:

looking forward for next dev-build!

@liamdebeasi
Copy link
Member

Thanks for the follow up. The behavior shown in the Mail app appears to be something separate than the card modal that you showed in #20700 (comment). I created a separate feature request for that here.

@liamdebeasi
Copy link
Member

Ok I fixed a couple things:

  1. I restored backdrop dismiss functionality. Native iOS seems to not use it on iPadOS with the card modals, but it is a breaking change for Ionic Framework so I decided to leave it in.
  2. You should be able to override the dimensions of the modal now.

Can you try the following dev build and let me know if it resolves the issue?

npm i @ionic/angular@5.1.0-dev.202003131952.9d9a02f

@DwieDima
Copy link
Contributor Author

DwieDima commented Mar 15, 2020

@liamdebeasi so much wow!

I've tested your dev build using iPhone and iPad view on chrome browser.
Now --width and --height are applied to modals. Backdrop dismiss also works.
here are some working samples using 600px for --width and --height:

also i discovered a bug, where card style modal gets incorrectly applied on iPad using

--width: 100%;
--height: 100%;

heres the sample:

thank you very much for your high commitment!

Update

pushing modals on android tablet view leads to overlapping backdrop background color.
Sample:

Update 2

I've deployed it on my device. Seems like the modal wont show the stacked card view, instead taking full height.
Modal is using 600px for width and height for cssClass (also without cssClass attribute, it makes no difference).
Sample:

@liamdebeasi
Copy link
Member

Thanks for the follow up.

also i discovered a bug, where card style modal gets incorrectly applied on iPad using

--width: 100%;
--height: 100%;

This is the correct behavior. 100% is going to make the dimensions of the modal be 100% of the parent container. In this case, that parent element is ion-app. If you want some offsets built in, you will need to add them yourself when specifying --width and --height.

pushing modals on android tablet view leads to overlapping backdrop background color.

If you feel this is a bug please open a separate issue as this does not appear to be related to the card modal updates.

I've deployed it on my device. Seems like the modal wont show the stacked card view, instead taking full height.

Fixed! Here's a dev build 5.1.0-dev.202003171608.9d9a02f.

This PR looks to be in pretty good shape, so I will get this reviewed and merged in as soon as I can. Thanks for all the testing and feedback!

@liamdebeasi
Copy link
Member

I am going to keep this issue open until I can get the PR merged in. Thanks!

@liamdebeasi liamdebeasi reopened this Mar 18, 2020
@liamdebeasi
Copy link
Member

Thanks for the issue (and for all the testing and feedback!). This has been resolved via #20750 and will be available in an upcoming release of Ionic Framework.

@ionitron-bot
Copy link

ionitron-bot bot commented Apr 24, 2020

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 Apr 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: core @ionic/core package type: bug a confirmed bug report
Projects
None yet
Development

No branches or pull requests

2 participants