Skip to content

Update modal to allow use of ios presentationStyle #1767

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

Closed
wants to merge 6 commits into from

Conversation

dottodot
Copy link
Contributor

@dottodot dottodot commented Mar 20, 2019

PR Checklist

What is the current behavior?

As per this issue #1709 it's currently not possible to use an ios popover style modal.

What is the new behavior?

I've added a couple of new options to ModalDialogOptions

  • sourceView - this is required as normally you would use viewContainerRef but if you want to attach a popover to a button for example you need to pass in the button element.
  • ios - allows you to use presentationStyle: UIModalPresentationStyle.Popover, the only caveat with this is you will need to install tns-platform-declarations to be able to use it.

You will also see I've added a closeModal() option. There were 2 reasons for this. Firstly if you click on the background the popover disappears but when you try to open another popover you get an error warning a modal already exists. So at the moment if a sourceView is present it will close any pre existing modal first before opening a new one.

The second issue I had is if I opened a popover then navigated back then went back to the same page to popover would reappear but not attached to the view it was previously attached to and with no content. So I made closeModal() public and call it when going back. i.e

    this.location.onPopState((resp: any) => {
      if (resp.pop) {
        this._modalService.closeModal();
      }
    });

I'm sure there's maybe a better way to resolve these issues but wasn't sure on that.

This solutions probably isn't perfect but it does work without any issues as far as I can tell, so think it's a good starting point at least.

Fixes/Implements/Closes #[Issue Number].
#1709

Marcus Williams added 4 commits March 20, 2019 18:18
@cla-bot cla-bot bot added the cla: yes label Mar 20, 2019
@ghost ghost added ♥ community PR and removed cla: yes labels Mar 20, 2019
@edusperoni
Copy link
Collaborator

edusperoni commented Mar 20, 2019

Why not use the new ShowModalOptions?

https://github.com/NativeScript/NativeScript/blob/master/tns-core-modules/ui/core/view-base/view-base.d.ts#L199

The old methods are deprecated at this point.

Since we already use a custom options object for angular (with view container ref and etc), it might be easier to pass something like

{...angularOptions, viewContainerRef: undefined, moduleRef: undefined, sourceView: undefined} plus the extra parameters needed (like closecallback). I'm partially suggesting this approach because it would make compatibility with nativescript-windowed-modal, or any plugin that extend modal options, that much easier.

Quick edit: I can help with this integration if you want to grant me access to the branch

}

@Injectable()
export class ModalDialogService {
private componentView: View;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this probably breaks multiple modals, as it stores only one componentview

Copy link
Contributor

@VladimirAmiorkov VladimirAmiorkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please read my comments and do the necessary changes. After that we can proceeded with merging this PR. Additionally you can follow this issue that is the source of the issues you have noticed. Those issues are are present in a non Angular project so it is best to resolve them not in nativescript-angular but rather in tns-core-modules. You could use the mentioned workaround of the popState so that it works in your project for the time being.

@@ -111,6 +121,13 @@ export class ModalDialogService {
});
}

public closeModal() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove this method. After locally testing and reviewing this PR I discovered a bug (NativeScript/NativeScript#7050) that when fixed will resolve this issue you are working around with this code so it is not needed.

} else {
(<any>parentView).showModal(componentView, context, closeCallback, fullscreen, animated, stretched);
}
this.componentView = componentView;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove this method. After locally testing and reviewing this PR I discovered a bug (NativeScript/NativeScript#7050) that when fixed will resolve this issue you are working around with this code so it is not needed.

@@ -159,7 +177,13 @@ export class ModalDialogService {

// TODO: remove <any> cast after https://github.com/NativeScript/NativeScript/pull/5734
// is in a published version of tns-core-modules.
(<any>parentView).showModal(componentView, context, closeCallback, fullscreen, animated, stretched);
if (ios) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As already mentioned, the code in the else is being deprecated and is not longer needed. You could simply do:

          let modalOptions: ShowModalOptions = {
               closeCallback: closeCallback,
               context: context,
               fullscreen: fullscreen,
               animated: animated,
               stretched: stretched,
               ios: ios
           };
            // TODO: remove <any> cast after https://github.com/NativeScript/NativeScript/pull/5734
            // is in a published version of tns-core-modules.
           (<any>parentView).showModal(componentView, modalOptions);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made the changes but based it on this PR #1769 as I think that's a better way to do it and will mean we don't have to keep adding options to match the core.

@cla-bot cla-bot bot added the cla: yes label Mar 21, 2019
@VladimirAmiorkov VladimirAmiorkov self-requested a review March 22, 2019 09:39
@VladimirAmiorkov
Copy link
Contributor

test

@VladimirAmiorkov
Copy link
Contributor

@dottodot @edusperoni Than you for your contribution. I have merged your PRs into a new one and added a test case for our e2e tests.

Closing in favor of #1771

@ghost ghost removed the ♥ community PR label Mar 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants