Enable 24-hr formatting for Android TimePicker #654

Merged
merged 1 commit into from Jan 11, 2017

Conversation

Projects
None yet
6 participants
@hardcodet
Contributor

hardcodet commented Dec 16, 2016

Description of Change

The renderer for the Android time picker opens the time picker dialog with a hardcoded 12-hr setting despite the fact the the platform allows to determine whether 24 hr time formatting was configured or not.

Bugs Fixed

Time picker should respect the 12hr/24hr setting of the user on Android.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense

Enable 24-hr time formatting depending on Android system settings instead of the hard coded 12-hour setting.

Enable 24-hr formatting for Android TimePicker
Enable 24-hr time formatting depending on Android system settings instead of the hard coded 12-hour setting.
@dnfclas

This comment has been minimized.

Show comment
Hide comment
@dnfclas

dnfclas Dec 16, 2016

Hi @hardcodet, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. Real humans will now evaluate your PR.

TTYL, DNFBOT;

dnfclas commented Dec 16, 2016

Hi @hardcodet, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. Real humans will now evaluate your PR.

TTYL, DNFBOT;

@@ -92,7 +93,8 @@ void OnClick()
TimePicker view = Element;
ElementController.SetValueFromRenderer(VisualElement.IsFocusedPropertyKey, true);
- _dialog = new TimePickerDialog(Context, this, view.Time.Hours, view.Time.Minutes, false);
+ bool is24HourFormat = DateFormat.Is24HourFormat(Context);

This comment has been minimized.

@adrianknight89

adrianknight89 Dec 16, 2016

Contributor

Instead of creating a local variable, you could pass the method call in the constructor.

@adrianknight89

adrianknight89 Dec 16, 2016

Contributor

Instead of creating a local variable, you could pass the method call in the constructor.

This comment has been minimized.

@hardcodet

hardcodet Dec 18, 2016

Contributor

I could, but how would that improve readability of the code?

@hardcodet

hardcodet Dec 18, 2016

Contributor

I could, but how would that improve readability of the code?

This comment has been minimized.

@adrianknight89

adrianknight89 Dec 18, 2016

Contributor

I suppose it's a matter of preference. You're calling a method named Is24HourFormat and declaring a variable with the same name. From the method name , we could infer the same thing.

@adrianknight89

adrianknight89 Dec 18, 2016

Contributor

I suppose it's a matter of preference. You're calling a method named Is24HourFormat and declaring a variable with the same name. From the method name , we could infer the same thing.

This comment has been minimized.

@hardcodet

hardcodet Dec 19, 2016

Contributor

I was never a fan of nesting method calls within method calls, for reasons of readability and ease of debugging. I don't have strong feeling here though, it's not my code after all - I can change this tomorrow.

@hardcodet

hardcodet Dec 19, 2016

Contributor

I was never a fan of nesting method calls within method calls, for reasons of readability and ease of debugging. I don't have strong feeling here though, it's not my code after all - I can change this tomorrow.

This comment has been minimized.

@adrianknight89

adrianknight89 Dec 19, 2016

Contributor

I don't have strong feeling either. Perhaps the actual reviewers can comment.

@adrianknight89

adrianknight89 Dec 19, 2016

Contributor

I don't have strong feeling either. Perhaps the actual reviewers can comment.

@StephaneDelcroix

This comment has been minimized.

Show comment
Hide comment
@StephaneDelcroix

StephaneDelcroix Dec 19, 2016

Member

I think we have one or multiple issues open for this, and we should dig that out

Member

StephaneDelcroix commented Dec 19, 2016

I think we have one or multiple issues open for this, and we should dig that out

@rmarinho rmarinho merged commit 11b982e into xamarin:master Jan 11, 2017

6 checks passed

Android-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run Android 6.0.1 : Tests passed: 351, i…
Details
OSX-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: OSX Debug : Running
Details
Windows-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: Windows Debug : Tests passed: 3685, ignored: 10
Details
iOS10-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS10 : Tests passed: 34…
Details
iOS8-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified IOS8 : Tests passed: 344…
Details
iOS9-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS9 : Tests passed: 346…
Details

@samhouts samhouts added this to the 2.3.4 milestone Jun 27, 2018

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