FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat(view): added iOS parameter for modal presentation style by surdu · Pull Request #6409 · NativeScript/NativeScript · GitHub

feat(view): added iOS parameter for modal presentation style - #6409

Merged
vakrilov merged 12 commits into
NativeScript:masterfrom
surdu:modal-style
Dec 13, 2018
Merged

vakrilov merged 12 commits into
NativeScript:masterfrom
surdu:modal-style

Conversation

surdu commented Oct 13, 2018

Copy link
Copy Markdown
Contributor

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 added the ♥ community PR label Oct 13, 2018

surdu commented Oct 13, 2018

Copy link
Copy Markdown
Contributor Author

@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.

tsonevn requested a review from MartoYankov October 15, 2018 06:55

Copy link
Copy Markdown
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 self-assigned this Nov 20, 2018

surdu commented Nov 24, 2018
edited
Loading

Copy link
Copy Markdown
Contributor Author

@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 added in progress and removed ♥ community PR labels Nov 26, 2018

Copy link
Copy Markdown
Contributor

test

Copy link
Copy Markdown
Contributor

test

const view = firstAgrument instanceof ViewCommon
? firstAgrument : <ViewCommon>createViewFromEntry({ moduleName: firstAgrument });

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Note to self: removed iosOpts by mistake

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 commented Dec 7, 2018

Copy link
Copy Markdown
Contributor

test

Copy link
Copy Markdown
Contributor

test

Copy link
Copy Markdown
Contributor

test

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

lock Bot commented Jan 11, 2020

Copy link
Copy Markdown

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 Bot locked and limited conversation to collaborators Jan 11, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

7 participants


Back | FazBrowse Home | New Git URL