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

Dynamic change of minDate and maxDate is gving error when changed to future than previous maxDate value #3545

Closed
bibekgg opened this issue Jan 16, 2020 · 3 comments · Fixed by #3565

Comments

@bibekgg
Copy link

bibekgg commented Jan 16, 2020

Bug description:

When the maxDate and minDate value is changed to future than previous maxDate value then it is giving below error

Error: 'maxDate' [object Object] should be greater than 'minDate' [object Object]

Link to minimally-working StackBlitz that reproduces the issue:

https://stackblitz.com/edit/angular-a9rdje

Versions of Angular, ng-bootstrap and Bootstrap:

Using latest demo StackBlitz

@peterblazejewicz
Copy link
Contributor

@bibekgg thanks for the report!

@maxokorokov
so this is because of moving ranges and test performed against previous values (happens also if one moves range before current range.
Fixing with preflight check:

diff --git a/src/datepicker/datepicker.ts b/src/datepicker/datepicker.ts
index 96b7c7d..e2ca0f7 100644
--- a/src/datepicker/datepicker.ts
+++ b/src/datepicker/datepicker.ts
@@ -416,10 +416,39 @@ export class NgbDatepicker implements OnDestroy,
   }
 
   ngOnChanges(changes: SimpleChanges) {
-    ['dayTemplateData', 'displayMonths', 'markDisabled', 'firstDayOfWeek', 'navigation', 'minDate', 'maxDate',
-     'outsideDays']
-        .filter(input => input in changes)
-        .forEach(input => this._service[input] = this[input]);
+    type DateChangesType = {
+      currentValue: NgbDateStruct,
+      previousValue: NgbDateStruct,
+    };
+    const dates = ['minDate', 'maxDate'];
+    const props = ['dayTemplateData', 'displayMonths', 'markDisabled', 'firstDayOfWeek', 'navigation', 'outsideDays'];
+    if ('minDate' in changes && 'maxDate' in changes) {
+      const minDates: DateChangesType = {...changes.minDate};
+      const maxDates: DateChangesType = {...changes.maxDate};
+      const dateValues = {
+        currentMinDate: NgbDate.from(minDates.currentValue),
+        previousMinDate: NgbDate.from(minDates.previousValue),
+        currentMaxDate: NgbDate.from(maxDates.currentValue),
+      };
+      // moving range back
+      if (dateValues.currentMinDate.before(dateValues.previousMinDate)) {
+        // moving max date past previous min date
+        if (dateValues.currentMaxDate.after(dateValues.previousMinDate)) {
+          props.push(...dates.reverse());
+          // moving max date back past previous min date
+        } else {
+          // default
+          props.push(...dates);
+        }
+        // moving range forward
+      } else {
+        props.push(...dates.reverse());
+      }
+    } else {
+      // partial dates update
+      props.push(...dates);
+    }
+    props.filter(input => input in changes).forEach(input => this._service[input] = this[input]);
 
     if ('startDate' in changes) {
       const {currentValue, previousValue} = changes.startDate;

Thoughts?

@maxokorokov maxokorokov added this to the 5.2.2 milestone Jan 27, 2020
maxokorokov added a commit to maxokorokov/ng-bootstrap that referenced this issue Jan 27, 2020
Previously internally calling

```ts
service.minDate = ...;
service.maxDate = ...;
```
resulted in two observable emissions, one per input change.

Now the API is changed to:

```ts
service.set({minDate: ..., maxDate: ...})
```

which results in a single emission for any number of inputs changed at the same time.

Fixes ng-bootstrap#3545
maxokorokov added a commit to maxokorokov/ng-bootstrap that referenced this issue Jan 27, 2020
Previously internally calling

```ts
service.minDate = ...;
service.maxDate = ...;
```
resulted in two observable emissions, one per input change.

Now the API is changed to:

```ts
service.set({minDate: ..., maxDate: ...})
```

which results in a single emission for any number of inputs changed at the same time.

Fixes ng-bootstrap#3545
@maxokorokov
Copy link
Member

maxokorokov commented Jan 27, 2020

@peterblazejewicz thanks for the idea, but this was due to an unfortunate datepicker service API design right from the start... opened a PR to fix it. Good thing all the changes are internal...

benouat pushed a commit that referenced this issue Feb 7, 2020
Previously internally calling

```ts
service.minDate = ...;
service.maxDate = ...;
```
resulted in two observable emissions, one per input change.

Now the API is changed to:

```ts
service.set({minDate: ..., maxDate: ...})
```

which results in a single emission for any number of inputs changed at the same time.

Fixes #3545
@bchowlam
Copy link

bchowlam commented Jul 27, 2020

@benouat Is this issue resolved. I am still facing the issue. Able to see and change the dates up to maxDate but form is invalid with previous maxDate validations on routing.

Please let me know how can we fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants