-
Notifications
You must be signed in to change notification settings - Fork 25k
[Android] Add TimePicker modes #12384
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
Conversation
|
Any progress on this? |
|
I merged master into my branch. Everything is ready on my side, I'm waiting for reviews / merge. |
|
Great, waiting for this change!For me, it's crazy to use different styles for date picker and time picker~ |
|
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. |
|
@faceach I can check if it's possible and propose a new PR for this. |
|
I don't know why I closed this.. Missclicked I guess. I reopen. |
|
cc / @mkonicek @ericvicenti |
|
I messed up with the branches and hard pushed another PR on top of this one.. :( |
|
PR updated and ready to be merged! |
|
@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. |
740302c to
1d6f7b2
Compare
|
Rebased against master. @mkonicek are you a good reviewer for this PR? |
| <TouchableWithoutFeedback | ||
| onPress={this.showPicker.bind(this, 'simple', {})}> | ||
| <Text style={styles.text}>{this.state.simpleText}</Text> | ||
| </TouchableWithoutFeedback> |
There was a problem hiding this comment.
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'})}>
|
@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. |
|
Rebased. Maybe I will have a little more chance with @satya164 ? :) |
|
@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( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 @@ | |||
| /** | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to stay iso with the DatePicker implementation:
https://github.com/Kerumen/react-native/blob/4e58d510958a96a6d307d7b0ba3bd8b47f70c563/ReactAndroid/src/main/java/com/facebook/react/modules/datepicker/DatePickerMode.java
|
@satya164 I updated the PR. Tell me if inlining the constants is really necessary. |
|
@facebook-github-bot shipit |
facebook-github-bot
left a comment
There was a problem hiding this 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.
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:  On **Android >= 5**, with the `spinner` mode:  And with the `clock` mode, the default:  Closes facebook/react-native#12384 Differential Revision: D6006689 Pulled By: hramos fbshipit-source-id: fcd37c867c4061b9982b1687f2c10211e54df7cf
Motivation
In the spirit of #10932, I added the
modeoption to theTimePickerAndroid API.There is only one mode available for Android < 5, the
spinnerone.If we are on Android >= 5 we can choose between
spinnerorclock. If we specifydefaultit will use the default of the current Android version.Test Plan
On Android < 5, whatever we choose it will be this:

On Android >= 5, with the

spinnermode:And with the

clockmode, the default: