NativeScript / nativescript-datetimepicker

Plugin with date and time picking fields
Apache License 2.0
27 stars 26 forks source link

Add a cancelled event to the date/time picker #44

Closed yassilah closed 4 years ago

yassilah commented 5 years ago

PR Checklist

What is the current behavior?

There is no way to know if the user cancelled or confirmed.

What is the new behavior?

A "cancelled" event is emitted when the return value of the picker is null.

Fixes #41.

cla-bot[bot] commented 5 years ago

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign the CLA at https://www.nativescript.org/cla. CLA has not been signed by users: @yassipad. After signing the CLA, you can ask me to recheck this PR by posting @cla-bot check as a comment to the PR.

yassilah commented 5 years ago

@cla-bot check

cla-bot[bot] commented 5 years ago

The cla-bot has been summoned, and re-checked this pull request!

elena-p commented 5 years ago

@yassipad,

Could you review and fix the lint errors - https://travis-ci.org/NativeScript/nativescript-datetimepicker/jobs/575108066?

cla-bot[bot] commented 5 years ago

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: yassipad. This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails
yassilah commented 5 years ago

@cla-bot check

cla-bot[bot] commented 5 years ago

The cla-bot has been summoned, and re-checked this pull request!

tgpetrov commented 5 years ago

Hi @yassipad I agree that such event would be useful to determine for sure if a user pressed ok or not. My only concern is that now we would not raise the datePickerClosed (and timePickerClosed) event (as the picker is also closed when the user pressed cancel). Maybe we can leave the datePickerClosed (and timePickerClosed) event as it is and add datePickerCancelled and datePickerConfirmed events to differentiate between the press of OK and cancellation. What do you think?

yassilah commented 5 years ago

Hi, Yes this could work :) But couldn’t we just emit both events cancel+closed at the same time?

tgpetrov commented 5 years ago

Yes, that's exactly what I mean - two events at the same - cancel+close vs confirm+close. Something like:

            .then((result: Date) => {
                const args = <EventData>{
                    eventName: DatePickerFieldBase.datePickerCancelledEvent,
                    object: this
                };

                if (result) {
                    this.date = result;
                    args.eventName = DatePickerFieldBase.datePickerConfirmedEvent;
                }
                this.notify(args);

                let closedArgs = <EventData>{
                    eventName: DatePickerFieldBase.datePickerClosedEvent,
                    object: this
                };
                this.notify(closedArgs);
            })
yassilah commented 5 years ago

Yes, great, should we send the closed event first or after the cancelled/confirmed event?

tgpetrov commented 5 years ago

It sounds more logical to me to send the confirm/cancel first and then the close event.

elena-p commented 4 years ago

@yassipad, Thank you for your pull request and your interest to contribute to the {N} community! However, there hasn't been any activity on this PR for more than a month and I will close it. Please, open a new PR when you have handled the comments above.