Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

Added a Flow Direction Property to action sheets and alert dialogs #11679

Merged
merged 16 commits into from
Dec 3, 2020

Conversation

memu8
Copy link
Contributor

@memu8 memu8 commented Aug 6, 2020

Description of Change

Added a Flow Direction Property to action sheets and alert dialogs. Also, changed the default flow direction of action sheets and alert dialogs to follow the page's effective flow direction.

Issues Resolved

API Changes

Added:

  • FlowDirection AlertSheetArguments.FlowDirection { get; set; }
  • DisplayActionSheet(string title, string cancel, string destruction, FlowDirection flowDirection, params string[] buttons)
  • DisplayAlert(string title, string message, string cancel, FlowDirection flowDirection)

Platforms Affected

  • Android
  • UWP

Behavioral/Visual Changes

The action sheets and alert dialogs may change depending on the flow direction of the parent page.

Before/After Screenshots

Before (no flow direction):

Screenshot 2020-08-06 at 1 13 15 PM
Screenshot 2020-08-06 at 1 13 48 PM

After (RTL flow direction):

Screenshot 2020-08-06 at 1 13 33 PM
Screenshot 2020-08-06 at 1 14 08 PM

Testing Procedure

Create an alert dialog and action sheet on Android and UWP and set the FlowDirection property to right-to-left and left-to-right to see if the alert dialog and actions sheet follows the flow direction properly. Also, can play around with changing the phone culture to right-to-left and left-to-right languages and see if the default flow direction of the alerts and action sheets follow the phone setting properly.

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)

@samhouts samhouts changed the title Fix 2448 Added a Flow Direction Property to action sheets and alert dialogs Aug 6, 2020
Copy link
Contributor

@hartez hartez left a comment

Choose a reason for hiding this comment

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

Review the option to use EffectiveFlowDirection to make the code more concise.

Comment on lines 568 to 590
if (options.FlowDirection == FlowDirection.MatchParent)
{
if (sender.FlowDirection == FlowDirection.RightToLeft)
options.FlowDirection = FlowDirection.RightToLeft;
else if (sender.FlowDirection == FlowDirection.LeftToRight)
options.FlowDirection = FlowDirection.LeftToRight;
else
{
if (Device.FlowDirection == FlowDirection.RightToLeft)
options.FlowDirection = FlowDirection.RightToLeft;
else if (Device.FlowDirection == FlowDirection.LeftToRight)
options.FlowDirection = FlowDirection.LeftToRight;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of checking the FlowDirection values for the Page (and then the Device) here, I think you can just use the Page's EffectiveFlowDirection - it should already be set to either RightToLeft or LeftToRight (depending on its FlowDirection settings.

Comment on lines 145 to 167
if (sender.FlowDirection == FlowDirection.LeftToRight)
{
layoutDirection = LayoutDirection.Ltr;
textDirection = TextDirection.Ltr;
}
else if (sender.FlowDirection == FlowDirection.RightToLeft)
{
layoutDirection = LayoutDirection.Rtl;
textDirection = TextDirection.Rtl;
}
else
{
if (Device.FlowDirection == FlowDirection.LeftToRight)
{
layoutDirection = LayoutDirection.Ltr;
textDirection = TextDirection.Ltr;
}
else if (Device.FlowDirection == FlowDirection.RightToLeft)
{
layoutDirection = LayoutDirection.Rtl;
textDirection = TextDirection.Rtl;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing as the UWP implementation - I think you can just look at the Page's EffectiveFlowDirection here instead of having to check for each possible setting of FlowDirection.

@samhouts samhouts requested a review from hartez August 12, 2020 16:37
@samhouts samhouts added the API-change Heads-up to reviewers that this PR may contain an API change label Aug 12, 2020
Xamarin.Forms.Core/Page.cs Outdated Show resolved Hide resolved
Xamarin.Forms.Core/ActionSheetArguments.cs Outdated Show resolved Hide resolved
Xamarin.Forms.Platform.Android/PopupManager.cs Outdated Show resolved Hide resolved
Xamarin.Forms.Platform.Android/PopupManager.cs Outdated Show resolved Hide resolved
Xamarin.Forms.Platform.Android/PopupManager.cs Outdated Show resolved Hide resolved
Xamarin.Forms.Platform.Android/PopupManager.cs Outdated Show resolved Hide resolved
@PureWeen PureWeen added this to the 5.0.0 milestone Nov 5, 2020
@PureWeen PureWeen removed the request for review from StephaneDelcroix November 10, 2020 22:36
@rmarinho rmarinho added the blocker Issue blocks next stable release. Prioritize fixing and reviewing this issue. label Nov 16, 2020
@PureWeen PureWeen assigned jsuarezruiz and unassigned PureWeen Dec 3, 2020
@PureWeen PureWeen removed their request for review December 3, 2020 19:21
@PureWeen PureWeen merged commit 0b74649 into 5.0.0 Dec 3, 2020
@PureWeen PureWeen deleted the fix-2448 branch December 3, 2020 20:19
pictos pushed a commit to pictos/Xamarin.Forms that referenced this pull request Dec 30, 2020
…amarin#11679)

* added constructor actionsheet

* added flow direction to action sheets on Android

* added flow direction to alerts on Android

* added Android alert dialog flow direction

* UWP alert/actionsheet flow direction

* add alert dialog flow direction

* added flow direction to UWP alert and actionsheets

* fixes xamarin#2448

* fixes xamarin#2448

* fixes xamarin#2448

* fixes xamarin#2448

* fixes xamarin#2448

* fixes xamarin#2448

* fixes xamarin#2448

* Fix rebase errors

* - fix rebase

Co-authored-by: E.Z. Hart <hartez@gmail.com>
Co-authored-by: Shane Neuville <shneuvil@microsoft.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a/alerts a/l10n a/rtl API-change Heads-up to reviewers that this PR may contain an API change blocker Issue blocks next stable release. Prioritize fixing and reviewing this issue. Core p/Android p/UWP proposal-accepted t/bug 🐛 t/enhancement ➕
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DisplayAlert Title not respecting FlowDirection Setting FlowDirection of Alerts and ActionSheets
6 participants