Skip to content

Refactor modal options to expose current/future core options #1769

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 2 commits into from

Conversation

edusperoni
Copy link
Collaborator

PR Checklist

What is the current behavior?

The current exposed options for showModal differs from the options available in the core.

What is the new behavior?

All current and future non-angular options are exposed by leveraging Typescript's Pick type.

#1767 attempts to fix this, but another property has been exposed (android: any), so other PRs would be needed for every new property.

I'm asking for this PR to be considered over #1767

Fixes #1709.

@VladimirAmiorkov
Copy link
Contributor

VladimirAmiorkov commented Mar 22, 2019

Hi @edusperoni ,

Thank you for your contribution with this PR and the comments on the #1767 PR. As I see that your PR's code changes have been taken and added to the #1767 PR and as it also contains additional functionality in the form of the new sourceView property of the ModalDialogOptions that is required for some of the ios specific options like the popover to work I suggest we merge that PR and close this one.

@edusperoni
Copy link
Collaborator Author

I don't really care which PR is merged, but that PR is still using the any cast that should be removed and the commits are kind of messy.

@VladimirAmiorkov
Copy link
Contributor

@edusperoni yes I have noticed that, it also is missing a "test" case example. We are going to process it.

@VladimirAmiorkov
Copy link
Contributor

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.

modal presentationStyle
2 participants