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

We can now use a custom DatePicker component #115

Merged
merged 4 commits into from
Feb 13, 2018

Conversation

SudoPlz
Copy link
Contributor

@SudoPlz SudoPlz commented Dec 5, 2017

Need this to be able to replace DatePickerIOS w/ something like dgladkov/react-native-uncontrolled-date-picker-ios

Usage:

<TimerPicker
	pickerRefCb={(datePicker) => { this.datepickerRef = datePicker; }}
	customDatePickerIOS={Platform.OS === 'ios' ? UncontrolledDatePickerIOS : null}
	onConfirm={() => {
		if (this.datepickerRef && this.datepickerRef.getDate) {
			this.datepickerRef.getDate((datePicked) => {
				// do something with the datePicked Date obj
			});
		}
	}}
/>

    - Need this to be able to replace DatePickerIOS w/ something like
    dgladkov/react-native-uncontrolled-date-picker-ios
@SudoPlz
Copy link
Contributor Author

SudoPlz commented Dec 5, 2017

This allows us to use a custom date picker like https://github.com/dgladkov/react-native-uncontrolled-date-picker-ios.

Why? Because we need it to get around this: facebook/react-native#8169

@mmazzarolo
Copy link
Owner

Thanks @SudoPlz !
I'll wait a bit before merging this PR since the configuration of this component is getting a bit too crowded.
Let's see if anyone else is interested in this feature :)

@abarisic86
Copy link
Contributor

@SudoPlz is this tested with https://github.com/dgladkov/react-native-uncontrolled-date-picker-ios latest version? Because uncontrolled-date-picker-ios does not do anything with onDateChange method. Rather, it implements getDate() method that should be called inside onConfirm handler of the parent component.

@SudoPlz
Copy link
Contributor Author

SudoPlz commented Jan 30, 2018

@abarisic86 I'm using this PR w/ https://github.com/dgladkov/react-native-uncontrolled-date-picker-ios on production already.

Notice the pickerRefCb prop? I use that like so:

import UncontrolledDatePickerIOS from 'react-native-uncontrolled-date-picker-ios';

<TimerPicker
    pickerRefCb={(datePicker) => { this.datepickerRef = datePicker; }}
    neverDisableConfirmIOS
    customDatePickerIOS={Platform.OS === 'ios' ? UncontrolledDatePickerIOS : null}
    ...

and then onConfirm I do:

this.datepickerRef.getDate((date) => {
    // do something with the date
});

@abarisic86
Copy link
Contributor

@SudoPlz it only works if I set neverDisableConfirmIOS to true. If I don't, it never calls onConfirm function.

Can you explain this line?
<View onStartShouldSetResponderCapture={neverDisableConfirmIOS !== true ? this._handleUserTouchInit : null}>

@SudoPlz
Copy link
Contributor Author

SudoPlz commented Jan 31, 2018

Before my pull request, we used to disable the confirm button while the user was interacting with the picker.
onStartShouldSetResponderCapture={this._handleUserTouchInit} does exactly that.

Now because we'll be using https://github.com/dgladkov/react-native-uncontrolled-date-picker-ios we no longer need the confirm and cancel buttons to become disabled, thus we pass the neverDisableConfirmIOS flag as true.

Some people may still want that behaviour so it's optional.
You might want to keep that flag set to true on your case too.

@abarisic86
Copy link
Contributor

abarisic86 commented Jan 31, 2018

@SudoPlz Yes, the usage of this parameter is clear. But is setting neverDisableConfirmIOS = {true} necessary for custom date picker to work? As I mentioned before, _handleDateChange inside modal is never called thus if 'neverDisableConfirmIOS' is set to false userIsInteractingWithPicker is never set to false and you can't click on confirm button.

The proper solution would be to modify react-native-uncontrolled-date-picker-ios to call this._onDateChange function when native picker signals that date is changed. A provided date will be wrong sometimes (that is the bug we are trying to solve) but we need that event fired to reset userIsInteractingWithPicker prop.

@SudoPlz
Copy link
Contributor Author

SudoPlz commented Jan 31, 2018

Or quit using the userIsInteractingWithPicker functionality all together (which is a feature I added a long time ago to counter the exact problem that https://github.com/dgladkov/react-native-uncontrolled-date-picker-ios is solving now).

We don't need userIsInteractingWithPicker any more.
Also, yes, it's true, if you want to use a customDatePickerIOS you'll have to set neverDisableConfirmIOS to true as well.
I prefer using that flag, than getting any wrong results at all.

Any reason why you don't want to use that flag?

@abarisic86
Copy link
Contributor

abarisic86 commented Feb 1, 2018

Yes, I had to add neverDisableConfirmIOS and it will go to production in our app.

Option to disable confirm button while interacting with date picker is still useful to stop users that click on confirm before the picker is done spinning.

  1. User opens the modal
  2. Scroll few times, datePicker settles on some date (change recorded)
  3. Makes another scroll gesture and clicks on confirm before date picker settles
  4. Date will change to value selected at step 2

I admit it is just an edge case and it's not the way datePicker should be used. But, if we want to fix this component for good - this option should be covered.

If we leave it as is I would suggest:

  • there should be a note inside Readme.md with information that this won't work if neverDisableConfirmIOS isn't set to true
  • add an explanation how to use customDatePickerIOS (something similar to your first post here)
  • maybe move the code from line 155 to _handleUserTouchInit and use something like code below to indicate that if you are using custom datePicker userIsInteractingWithPicker should not change. In that case, this will work even if neverDisableConfirmIOS is set to false
 _handleUserTouchInit = () => {
    // custom date picker shouldn't change this param
    if(!customDatePickerIOS){
        this.setState({
            userIsInteractingWithPicker: true,
        })
    }
    return false;
};

@SudoPlz
Copy link
Contributor Author

SudoPlz commented Feb 1, 2018

@abarisic86 That's a good suggestion, I'll do that.

Also, are you sure your steps are correct?
I haven't tested this, but to me it sounds as if it should be that way:

  1. User opens the modal
  2. Scroll few times, datePicker settles on some date (change recorded)
  3. Makes another scroll gesture and clicks on confirm before date picker settles
  4. Date will change to value selected at step 2 .getDate will run, and whatever the current value of the spinning component is will be returned.

@SudoPlz
Copy link
Contributor Author

SudoPlz commented Feb 1, 2018

All set. @mmazzarolo Can you merge? This doesn't change anything to the existing usage, it only adds an extra feature that lot's of people require.

@@ -148,8 +162,9 @@ export default class CustomDatePickerIOS extends PureComponent {
>
<View style={[styles.datepickerContainer, datePickerContainerStyleIOS]}>
{customTitleContainerIOS || titleContainer}
<View onStartShouldSetResponderCapture={this._handleUserTouchInit}>
<DatePickerIOS
<View onStartShouldSetResponderCapture={neverDisableConfirmIOS !== true ? this._handleUserTouchInit : null}>
Copy link
Contributor

Choose a reason for hiding this comment

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

You should revert this to <View onStartShouldSetResponderCapture={this._handleUserTouchInit}>

Copy link
Contributor Author

@SudoPlz SudoPlz Feb 1, 2018

Choose a reason for hiding this comment

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

Hmm any reason why we should do that?
People may want to disable the confirmation buttons while the date picker is scrolling, even if they're not using a Custom date picker.

Can you think of any scenarios were that becomes a problem?

Copy link
Contributor

@abarisic86 abarisic86 Feb 1, 2018

Choose a reason for hiding this comment

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

Is there any reason to make this change? any scenario where we need this condition?

Although it doesn't generate any bugs:

  • neverDisableConfirmIOS feature works without this condition (when using default datepicker)
  • The main problem I see is readability. It's easier to follow code if we leave this condition out
  • "neverDisableConfirmIOS !== true" is confusing

So, can we leave it out?

@mmazzarolo
Copy link
Owner

Hey @SudoPlz and @abarisic86
Even if I din't post here I was following the issue, thanks for the discussion!

@SudoPlz could you address the last @abarisic86 suggestion before the merge?

@konstantin-mohin
Copy link

konstantin-mohin commented Feb 13, 2018

@mmazzarolo
in the _handleUserTouchInit method use this.props.customDatePickerIOS

@SudoPlz
Copy link
Contributor Author

SudoPlz commented Feb 13, 2018

Ahm, but I did make those changes back then.

I don't think it needs anything else, right?

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.

4 participants