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(view): added iOS parameter for modal presentation style #6409

Merged
merged 12 commits into from Dec 13, 2018

Conversation

surdu
Copy link
Contributor

@surdu surdu commented Oct 13, 2018

PR Checklist

What is the current behavior?

Other than fullscreen or not, there is no way to set the rest of modal presentation styles for a modal on iOS

What is the new behavior?

You can now pass an extra parameter called iOSPresentationStyle to .showModal()

Implements #6387

@ghost ghost added the ♥ community PR label Oct 13, 2018
@surdu
Copy link
Contributor Author

surdu commented Oct 13, 2018

@tsonevn It turns out this is not so easy as just passing a parameter to .showModal(). Some of the presentation styles require additional logic in order to make them work.

I've already implemented some default logic for UIModalPresentationPopover and while I believe some default logic can also be implemented for most of the otehrs, UIModalPresentationCustom might be a little bit complicated withouth returning the view controller to the user.

I will investigate further, but wanted to open the discussion here for further ideas.

@vakrilov
Copy link
Contributor

Hey there @surdu - thanks for you effort. Here are a couple of thoughts:

Definitions

First of all - all d.ts files should be clear of IOS or Android Specific classes(like UIModalPresentationStyle). The reason is that we don't want people to be forced to use tna-platform-declarations package in their apps.

Options

The showModal API already has 3 optional parameters and this is making it little hard to use. Adding one more optional param will amplify the problem (what if you want to pass only the last one).

My suggestion is to define a ShowModalOptions interface that you pass to the method. This way you can fill in only the options you care about. Furthermore you can have an ios and android portion of the interface that only deals with plat-spceific options (like UIModalPresentationStyle).

You can also have a callback in the options - which will be called if they are using the an option, that needs to give the control back to the user for additional setup of a view-controller for example.

PR

Seems that this PR is not as easy as originally thought. What do you think of moving the discussion back to the issue (#6387). We might pick only the UIModalPresentationPopover values that are most useful - no point of supporting all of them if they are not going to be used.

@vakrilov vakrilov self-assigned this Nov 20, 2018
@surdu
Copy link
Contributor Author

surdu commented Nov 24, 2018

@vakrilov I've made the changes that you requested.

Also wrote a test for my changes. I took another test for the modal and just changed the way the options are passed. The test is somewhat overkill, but I guess there was a good reason to implement the test I copied like this, so I went with it.

There is one thing that I want to do but I didn't manage to figure out how (probably can't be done at this stage of execution): I want to get styling informations about the Page that is about to be presented at this point. It would be nice to get the Page width and height expressed by the user is css and resize the pop-up accordingly. Also need the background color few lines bellow.

For the old version of showModal that receives the options as arguments, do you want to deprecate it or will they live alongside each other?

@ghost ghost added in progress and removed ♥ community PR labels Nov 26, 2018
@vakrilov
Copy link
Contributor

test

@dtopuzov
Copy link
Contributor

test


(<ViewCommon>view)._showNativeModalView(this, context, closeCallback, fullscreen, animated, stretched, iosOpts);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to self: removed iosOpts by mistake

@vakrilov vakrilov changed the title [DO NOT MERGE] feat(view): added iOS parameter for modal presentation style feat(view): added iOS parameter for modal presentation style Dec 6, 2018
@vakrilov
Copy link
Contributor

vakrilov commented Dec 7, 2018

test

@vakrilov
Copy link
Contributor

test

@vakrilov
Copy link
Contributor

test

@ghost ghost assigned SvetoslavTsenov Dec 13, 2018
@vakrilov vakrilov merged commit 540b2b4 into NativeScript:master Dec 13, 2018
@ghost ghost removed the in progress label Dec 13, 2018
@surdu surdu deleted the modal-style branch December 13, 2018 15:05
@MartoYankov MartoYankov added the docs needed Additional documentation on this issue/PR is needed label Dec 13, 2018
@tsonevn tsonevn requested review from tsonevn and removed request for tsonevn January 11, 2019 05:59
@tsonevn tsonevn self-assigned this Jan 11, 2019
@lock
Copy link

lock bot commented Jan 11, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Jan 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes ♥ community PR docs needed Additional documentation on this issue/PR is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants