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

Support Countdown Mode #351

Closed
wants to merge 3 commits into from
Closed

Conversation

chetstone
Copy link

Overview

In Countdown mode, there is a bug in the underlying library that causes the Confirm button of this component to not work when a value is first selected by the picker. You have to move the wheel at least twice. This PR implements the workaround suggested in that issue.

Although this fixes the problem for the main use case, it unfortunately does not fix it completely. There is a use case (hopefully rare) where, if the user spins the picker to 0 hours, 0 minutes, iOS will immediately advance the picker to 0 hours 1 minute and the Confirm button won't work the next time you move the picker.... it has to be moved at least twice. I haven't been able to figure out a workaround for this.

Test Plan

I modified the example component, adding a Duration (countdown) button for iOS only (the underlying component does not support countdown on Android.) I ran the example on an iOS device and it works fine. I also ran on an Android device to make sure I didn't break anything there.

Copy link
Owner

@mmazzarolo mmazzarolo left a comment

Choose a reason for hiding this comment

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

Hi @chetstone , thanks for submitting a PR!
I appreciate the work you did here, but for keeping a sane maintainability I'd prefer to have this issue solved directly in the community component instead of adding workarounds here.

example/App.js Outdated
? "Duration"
: pickerMode &&
pickerMode.charAt(0).toUpperCase() + pickerMode.slice(1)
}`}
Copy link
Owner

Choose a reason for hiding this comment

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

I'd prefer to keep the example as simple as possible. I think we can skip showing the countdown mode here.

@@ -68,11 +68,11 @@ export default class DateTimePickerModal extends React.PureComponent {
state = {
currentDate: this.props.date,
isPickerVisible: this.props.isVisible,
isPickerSpinning: false
isPickerSpinning: false,
initialized: false
Copy link
Owner

Choose a reason for hiding this comment

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

nit: isInitialized instead of initialized

componentDidUpdate(prevProps, prevState) {
/* console.log(
* `Did Update: Mode: ${this.props.mode}, Visible: ${this.props.isVisible}, prevVisible: ${prevProps.isVisible}`
* );*/
Copy link
Owner

Choose a reason for hiding this comment

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

Leftover?

!prevProps.isVisible
) {
setTimeout(() => this.setState({ initialized: true }), 0);
/* console.log("Visible");*/
Copy link
Owner

Choose a reason for hiding this comment

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

Leftover?

@chetstone
Copy link
Author

Thanks for looking at this. I understand why you don't want to merge it-- makes sense. I'll go over to the community and raise a ruckus and see if I can't get someone to fix it right. I don't think I have the native coding chops to root-cause it myself.

In the meantime, could we leave this PR open for now? If others need to use this I'd like for them to be able to find it. I've cleaned up the code as you suggested. I simplified the countdown button in the example but left it in for now because it is necessary to test my fix.

Thanks again.

@mmazzarolo
Copy link
Owner

In the meantime, could we leave this PR open for now?
Absolutely. Again, thanks for the support :)

@mmazzarolo mmazzarolo closed this Feb 16, 2020
@AManNamedGaurav
Copy link

@chetstone Just wanted to say thank you for adding this workaround. I've been stuck on this problem for a couple days now and now my app is working as it should! Thank you!

zamarlancer added a commit to zamarlancer/react-native-modal-datetime-picker that referenced this pull request Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants