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

[Android] Add TimePicker modes #12384

Closed
wants to merge 2 commits into from
Closed

Conversation

Kerumen
Copy link
Contributor

@Kerumen Kerumen commented Feb 14, 2017

Motivation

In the spirit of #10932, I added the mode option to the TimePicker Android API.
There is only one mode available for Android < 5, the spinner one.
If we are on Android >= 5 we can choose between spinner or clock. If we specify default it will use the default of the current Android version.

Test Plan

On Android < 5, whatever we choose it will be this:
screen shot 2017-02-14 at 17 05 44

On Android >= 5, with the spinner mode:
screen shot 2017-02-14 at 16 51 17

And with the clock mode, the default:
screen shot 2017-02-14 at 16 51 02

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. GH Review: review-needed labels Feb 14, 2017
@faceach
Copy link

faceach commented Mar 1, 2017

Any progress on this?

@Kerumen
Copy link
Contributor Author

Kerumen commented Mar 1, 2017

I merged master into my branch. Everything is ready on my side, I'm waiting for reviews / merge.

@faceach
Copy link

faceach commented Mar 1, 2017

Great, waiting for this change!For me, it's crazy to use different styles for date picker and time picker~

@faceach
Copy link

faceach commented Mar 1, 2017

One suggestion, could we also pass theme color to date/time picker? Then date/time picker components will have capability to change their background colors. As now they use cyan by default.

@Kerumen
Copy link
Contributor Author

Kerumen commented Mar 1, 2017

@faceach I can check if it's possible and propose a new PR for this.

@Kerumen
Copy link
Contributor Author

Kerumen commented Apr 6, 2017

I don't know why I closed this.. Missclicked I guess. I reopen.

@Kerumen Kerumen reopened this Apr 6, 2017
@cyprusglobe
Copy link

cc / @mkonicek @ericvicenti

@Kerumen
Copy link
Contributor Author

Kerumen commented Apr 10, 2017

I messed up with the branches and hard pushed another PR on top of this one.. :(
Will play with git reflog tomorrow to get back my previous work.

@Kerumen
Copy link
Contributor Author

Kerumen commented Apr 11, 2017

PR updated and ready to be merged!

@facebook-github-bot
Copy link
Contributor

@Kerumen I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

@Kerumen Kerumen force-pushed the master branch 2 times, most recently from 740302c to 1d6f7b2 Compare August 8, 2017 12:43
@Kerumen
Copy link
Contributor Author

Kerumen commented Aug 8, 2017

Rebased against master. @mkonicek are you a good reviewer for this PR?

@@ -59,6 +62,24 @@ class TimePickerAndroidExample extends React.Component {
onPress={this.showPicker.bind(this, 'simple', {})}>
<Text style={styles.text}>{this.state.simpleText}</Text>
</TouchableWithoutFeedback>

Choose a reason for hiding this comment

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

null: Parsing error: Expected corresponding JSX closing tag for

63 | {this.state.simpleText}
64 |

65 |
| ^
66 |
67 | <TouchableWithoutFeedback
68 | onPress={this.showPicker.bind(this, 'clock', {mode: 'clock'})}>

@facebook-github-bot
Copy link
Contributor

@Kerumen I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

@Kerumen
Copy link
Contributor Author

Kerumen commented Sep 8, 2017

Rebased. Maybe I will have a little more chance with @satya164 ? :)

@facebook-github-bot
Copy link
Contributor

@Kerumen I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

hour,
minute,
is24hour);
TimePickerDialog dialog = new DismissableTimePickerDialog(
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe do this in the last else block to avoid unnecessarily instantiating it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True! I change this :)

@@ -0,0 +1,17 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

I think better to inline this constants in the time picker module file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kerumen
Copy link
Contributor Author

Kerumen commented Oct 8, 2017

@satya164 I updated the PR. Tell me if inlining the constants is really necessary.
Thanks for your time!

@satya164
Copy link
Contributor

satya164 commented Oct 8, 2017

@facebook-github-bot shipit

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Oct 8, 2017
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@satya164 is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

rozele pushed a commit to microsoft/react-native-windows that referenced this pull request Nov 7, 2017
Summary:
In the spirit of #10932, I added the `mode` option to the `TimePicker` Android API.
There is only one mode available for **Android < 5**, the `spinner` one.
If we are on **Android >= 5** we can choose between `spinner` or `clock`. If we specify `default` it will use the default of the current Android version.

On **Android < 5**, whatever we choose it will be this:
![screen shot 2017-02-14 at 17 05 44](https://cloud.githubusercontent.com/assets/5436545/22937805/024ec67e-f2da-11e6-8b32-a680d9bc2247.png)

On **Android >= 5**, with the `spinner` mode:
![screen shot 2017-02-14 at 16 51 17](https://cloud.githubusercontent.com/assets/5436545/22937803/024e0bbc-f2da-11e6-9f4b-26102ff2eeac.png)

And with the `clock` mode, the default:
![screen shot 2017-02-14 at 16 51 02](https://cloud.githubusercontent.com/assets/5436545/22937804/024e64e0-f2da-11e6-9911-4135049f4726.png)
Closes facebook/react-native#12384

Differential Revision: D6006689

Pulled By: hramos

fbshipit-source-id: fcd37c867c4061b9982b1687f2c10211e54df7cf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Import Started This pull request has been imported. This does not imply the PR has been approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants