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

[Enhancement][iOS] Support UIModalPresentationStyle.FormSheet #3119

Merged
merged 6 commits into from Jul 10, 2018

Conversation

Projects
None yet
5 participants
@jfversluis
Contributor

jfversluis commented Jun 22, 2018

Description of Change

Added an enum and implemented logic in the modal presentation on the iPad. As per #1726 you are now able to show a modal popup as a form sheet on the iPad. By default, it will still show fullscreen, but you now have the possibility of setting it to "FormSheet" which causes a smaller sized overlay to show on the iPad.

Had to hook into the Platform.PresentModal, that was the only place where setting the UIViewController.ModalPresentationStyle had effect for some reason. Ideally, I would have wanted this in the renderer(?). If anyone sees how to do it there, please let me know.

No tests were added since this is a visual change only.

Issues Resolved

API Changes

Added:

  • Page.On.SetModalPresentationStyle(UIModalPresentationStyle.FormSheet)

Platforms Affected

  • iOS (iPad only)

Behavioral/Visual Changes

When upgrading no change should happen, only when the user explicitly sets the modal presentation style.

PR Checklist

  • Has automated tests
  • Rebased on top of the target branch at time of PR
  • Changes adhere to coding standard

jfversluis added some commits Jun 6, 2018

Merge pull request #3 from xamarin/master
Merge from origin
First attempt at #1726
Implemented enum and hooked into modal presentation to set the right flag whenever a screen is presented modally on the iPad

@jfversluis jfversluis changed the title from [iOS] Support UIModalPresentationStyle.FormSheet to [Enhancement][iOS] Support UIModalPresentationStyle.FormSheet Jun 22, 2018

@samhouts samhouts added this to In Review in vNext+1 (master) Jun 22, 2018

@samhouts samhouts requested review from rmarinho and PureWeen Jun 25, 2018

@jfversluis

This comment has been minimized.

Contributor

jfversluis commented Jun 26, 2018

Anything I broke with the tests?

@rmarinho

This comment has been minimized.

Member

rmarinho commented Jun 26, 2018

@jfversluis nah just UITests acting up

namespace Xamarin.Forms.Controls.GalleryPages.PlatformSpecificsGalleries
{
public class ModalFormSheetPageiOS : ContentPage

This comment has been minimized.

@PureWeen

PureWeen Jul 6, 2018

Contributor

Can this page have a button that will let the user go back to the Control Gallery? If you click on other platform specific gallery pages you'll notice each one has a button for "Back to gallery" otherwise the user is just kind of stuck

This comment has been minimized.

@jfversluis

jfversluis Jul 7, 2018

Contributor

Ah of course, was too quick on the actual code, forgot to tidy this one up

This comment has been minimized.

@jfversluis

jfversluis Jul 7, 2018

Contributor

Done

@@ -462,6 +463,9 @@ static void PresentPopUp(UIWindow window, UIAlertController alert, ActionSheetAr
var wrapper = new ModalWrapper(modalRenderer);
if (Device.Idiom == TargetIdiom.Tablet && modal.OnThisPlatform().ModalPresentationStyle() == PlatformConfiguration.iOSSpecific.UIModalPresentationStyle.FormSheet)

This comment has been minimized.

@PureWeen

PureWeen Jul 6, 2018

Contributor

Thoughts on moving this check into the constructor with something like this

var elementConfiguration = modal.Element as IElementConfiguration<Page>;
			if(elementConfiguration?.On<PlatformConfiguration.iOS>().ModalPresentationStyle() == PlatformConfiguration.iOSSpecific.UIModalPresentationStyle.FormSheet)
				ModalPresentationStyle = UIKit.UIModalPresentationStyle.FormSheet;

I'm thinking having this aspect of the setup not being a burden on the caller might be more reliable down the road. Thoughts?

This comment has been minimized.

@jfversluis

jfversluis Jul 7, 2018

Contributor

I see your point and might be a nice optimization, but I don't see where you'd want to put it/ Constructor of Platform?

This comment has been minimized.

@PureWeen

PureWeen Jul 9, 2018

Contributor

My thinking was to just put it inside the ModalWrapper constructor.

This comment has been minimized.

@jfversluis

jfversluis Jul 9, 2018

Contributor

Ah of course, wow. Overlooked that. That's what you get for coding over the weekend. Absolutely right on moving it to there and not have an additional check when the actual modal is shown. Will update this in a second.

@jfversluis

This comment has been minimized.

Contributor

jfversluis commented Jul 9, 2018

@PureWeen updated according to comments!

@PureWeen

One last change to remove the no longer needed namespace and then we should be good to merge

@@ -7,6 +7,7 @@
using UIKit;
using RectangleF = CoreGraphics.CGRect;
using Xamarin.Forms.Internals;
using Xamarin.Forms.PlatformConfiguration.iOSSpecific;

This comment has been minimized.

@PureWeen

PureWeen Jul 10, 2018

Contributor

This can be removed now

@jfversluis

This comment has been minimized.

Contributor

jfversluis commented Jul 10, 2018

Good catch, done!

@PureWeen PureWeen added the p/iOS 🍎 label Jul 10, 2018

@PureWeen PureWeen merged commit 271cb30 into xamarin:master Jul 10, 2018

1 check passed

license/cla All CLA requirements met.
Details

vNext+1 (master) automation moved this from In Review to Done Jul 10, 2018

PureWeen added a commit that referenced this pull request Aug 28, 2018

[Enhancement][iOS] Support UIModalPresentationStyle.FormSheet (#3119)
* First attempt at #1726

Implemented enum and hooked into modal presentation to set the right flag whenever a screen is presented modally on the iPad

* Added back to gallery button on sample app

* Moved setting UIModalPresentationStyle to ModalWrapper ctor

* Removed unused usings

@samhouts samhouts added this to the 3.3.0 milestone Sep 20, 2018

@samhouts samhouts removed this from Done in vNext+1 (master) Sep 21, 2018

@samhouts samhouts added the F100 label Nov 2, 2018

@samhouts samhouts removed this from Done in vCurrent (Target 3.4.0) Nov 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment