-
Notifications
You must be signed in to change notification settings - Fork 125
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
Proposal to merge DatePickeriOS and DatePickerAndroid #85
Conversation
Thanks for writing this up! From my perspective at Facebook, I trust the community to come up with an API that people like for this. We'll be happy to remove the existing APIs from React Native once a component exists that people are happy with, feels solid, and has a well defined migration path from the existing APIs. cc @EvanBacon who may have thoughts on this from the Expo side and how a web implementation could be supported with this API. |
function DateTimePickerAndroid({ mode, onDateChange, value, ...props }) { | ||
const Selector = mode === 'time' ? TimePickerAndroid : DatePickerAndroid; | ||
|
||
Selector.open({ ...value, ...props }) // act on thenable with `onDateChange` |
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've had trouble in the past with calling APIs that show modal UI from a render function. For example, invoking ActionSheets or Alerts from the render function can result in iOS opening multiple modal UIs on top of each other. Whatever strategy you choose should mitigate this, and probably means maintaining some internal state that can tell whether the UI being invoked programmatically is already shown.
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.
(It's possible this concern is only valid for iOS and doesn't apply to the Android APIs.)
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.
Yep, React reserves the right to call render as many times as it wants
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.
Thanks for this callout, I'll give it some thoughts and check the default behavior on both iOS and Android to see what happens in either case.
#### value | ||
|
||
Both `DatePickerIOS` and `DatePickerAndroid` use `date` whereas `TimePickerAndroid` requires an `hour` and | ||
`minute` property. The proposal is to change this property to `value {Date, Number}`. Where `{Number}` is |
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.
Can you clarify for me what value {Date, Number}
is intended to communicate? Does that mean value
can be either Date
object or a Number
? Or does it mean it's an object containing those two fields?
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.
@mmcknett From above:
Finally, iOS allows
date
to be atimestamp
orDate
object.DatePickerAndroid
matches this
implementation. TheTimePickerAndroid
however requires anumber
to be passed to thehour
andminute
props.
I think that answers 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.
Ah, got it, it's the same as date
in the iOS version, but with additional genericness in the name so that it can be used to pass a countdown value (as described below) as well. Might be worth clarifying that in "Where {Number}
is..."
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.
Yeah, I was looking for the best way to convey this, but glad the additional comments make it clear. If you want me to change this to some other way to denote a Set
of options, I'll change it :)
- `date` (new default for both android and iOS) | ||
- `time` | ||
- `datetime (iOS)` | ||
- `countdown (iOS)` |
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.
Although "countdown" is technically one of the available modes of the UIDatePicker
, I'm not sure it belongs here. Two things combine to make it weird in this context: 1) it's iOS-only; 2) it represents a time span, not a point in time. How crazy would it be to create a separate CountdownPickerIOS
RN component to encapsulate just a UIDatePicker
in countDownTimer
mode, with an interface to set the minutes on the countdown timer? That would free DateTimePicker
from having the responsibility of representing a concept that is distinct from a date or time.
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.
That could be a path forward, I consider this outside the scope of this proposal though. The option currently exists for native iOS but it is not fully implemented (wrt configurability). It is sort of usable right now, without it being documented completely. https://github.com/react-native-community/discussions-and-proposals/pull/85/files/3255b2c845018924c2d0a6978bcf91f92893f736#diff-cfdf3e4461139c8caebcec975f57c066R163 It is here for completeness. As it stands the new implementation should not implement specific logic around this.
I got excited about time & date controls and totally forgot to say: thanks for this proposal, @Swaagie! I love that we're talking about fewer platform-specific and more cross-platform controls as the effort to decrease overall dependency sizes decreases too. On to Lean Core! |
As we decided to do this work in a repository outside of React Native, and removing the existing implementations from RN as part of Lean Core, I'm going to close this PR as it doesn't need to be merged here since we only merge things related to RN itself. |
Public writing about the effort https://godaddy.github.io/2019/06/17/react-native-community-contribution-datetimepicker-component/ |
This proposal describes an approach to merge the date- and timepicker for both platforms.