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

fix(datepicker): no longer trigger onChange when the value is equivalent #2668

Merged
merged 1 commit into from
Sep 13, 2018

Conversation

divdavem
Copy link
Member

Before submitting a pull request, please make sure you have at least performed the following:

  • read and followed the CONTRIBUTING.md guide.
  • built and tested the changes locally.
  • added/updated any applicable tests.
  • added/updated any applicable API documentation.
  • added/updated any applicable demos.

Copy link
Member

@maxokorokov maxokorokov left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix!

Left one comment about state. WDYT?

@@ -149,6 +149,7 @@ export class NgbDatepicker implements OnDestroy,
OnChanges, OnInit, ControlValueAccessor {
model: DatepickerViewModel;

private _controlValue: NgbDate;
Copy link
Member

Choose a reason for hiding this comment

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

Do you mind if we move this to the datepicker service?

Not sure it it makes a huge difference, but the idea was to keep any state away from component.

I would have added an option:

this._service.select(..., {fromControl: true});

in this case the service's select would to something like:

if (isChangedDate(this._state.selectedDate, selectedDate)) {
        this._nextState({selectedDate, controlValue: options.fromControl ? selectedDate : undefined});
      }

P.S. names formControl and controlValue are just examples

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for your comment! It is a good idea! I will update my PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, in fact, as discussed, I think it is better as it is now (as we need to update controlValue again when calling onChange).

Copy link
Member

Choose a reason for hiding this comment

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

Yep, as discussed, simpler to leave it as is

@divdavem
Copy link
Member Author

@maxokorokov Thank you for your review!

@maxokorokov maxokorokov merged commit f8977be into ng-bootstrap:master Sep 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants