bug(#3497): fix calendar years dropdown when min/max = today#3508
Conversation
✅ Deploy Preview for goa-design-2 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
61bbe60 to
8d5ac91
Compare
8d5ac91 to
067f7b6
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses Bug #3497 where the calendar year dropdown can become empty when min/max are set (notably from the Angular wrapper), and adds demo routes in the PR apps to reproduce/verify the behavior.
Changes:
- Angular calendar wrapper now binds
min/maxas ISO strings instead of relying onDate.toString(). - Web component calendar adds warnings when
min/maxparse as invalid dates. - Adds Bug 3497 reproduction pages/routes in both the React and Angular PR apps.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| libs/web-components/src/components/calendar/Calendar.svelte | Adds console warnings for invalid min/max inputs while recalculating year options. |
| libs/angular-components/src/lib/components/calendar/calendar.ts | Updates min/max attribute binding to ISO strings; adds initialization logging. |
| libs/angular-components/src/lib/components/calendar/calendar.spec.ts | Updates unit assertions to match ISO string binding for min/max. |
| apps/prs/react/src/routes/bugs/bug3497.tsx | Adds a React reproduction route for the bug scenario. |
| apps/prs/react/src/main.tsx | Registers the new React bug route and reformats some existing routes. |
| apps/prs/react/src/app/app.tsx | Adds a side-menu link to the new React bug route. |
| apps/prs/angular/src/routes/bugs/3497/bug3497.component.ts | Adds an Angular reproduction component for the bug scenario. |
| apps/prs/angular/src/routes/bugs/3497/bug3497.component.html | Adds the Angular reproduction template. |
| apps/prs/angular/src/app/app.routes.ts | Registers the new Angular bug route. |
| apps/prs/angular/src/app/app.component.html | Adds a side-menu link to the new Angular bug route. |
Comments suppressed due to low confidence (1)
libs/web-components/src/components/calendar/Calendar.svelte:96
- The new warnings detect invalid
min/max, but the component still uses_min.year/_max.yearto computeyearCount. When either date is invalid,yearCountbecomesNaN(coerces to 0) and the year dropdown remains empty. Consider falling back to the default min/max (or the last known valid values) when!_min.isValid()/!_max.isValid(), and guard against non-positive/invalidyearCountbefore building_years.
_min = min ? new CalendarDate(min) : new CalendarDate().addYears(-5);
_max = max ? new CalendarDate(max) : new CalendarDate().addYears(+5);
if (!_min.isValid()) {
console.warn(`Invalid min date: ${min}.`);
}
if (!_max.isValid()) {
console.warn(`Invalid max date: ${max}.`);
}
// Update years list based on new min/max
const yearStart = _min.year;
const yearCount = _max.year - yearStart + 1;
_years = Array.from({ length: yearCount }, (_, i) => `${yearStart + i}`);
You can also share your feedback on Copilot code review. Take the survey.
067f7b6 to
a798950
Compare
bd4d00d to
e8ff723
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
libs/web-components/src/components/calendar/Calendar.svelte:116
- After detecting an invalid
min/maxyou still continue using_min/_maxto computeyearStart/yearCountand for boundary comparisons. With an invalid date this can produce an empty_yearslist and can also lead to runtime errors when callingisBefore/isAfteragainst an invalidDate. Consider falling back to the default range (e.g., ±5 years from today) or skipping the range update when_min/_maxare invalid (and also handlemin > max).
_min = min ? new CalendarDate(min) : new CalendarDate().addYears(-5);
if (!_min.isValid()) {
console.error(
`goa-calendar ${name ?? testid}: Invalid min date provided: ${min}.`,
);
}
_max = max ? new CalendarDate(max) : new CalendarDate().addYears(+5);
if (!_max.isValid()) {
console.error(
`goa-calendar ${name ?? testid}: Invalid max date provided: ${max}.`,
);
}
// Update years list based on new min/max
const yearStart = _min.year;
const yearCount = _max.year - yearStart + 1;
_years = Array.from({ length: yearCount }, (_, i) => `${yearStart + i}`);
// Adjust calendar if it's outside the new min/max range
if (_calendarDate) {
if (_calendarDate.isBefore(_min)) {
_calendarDate = new CalendarDate(_min);
} else if (_calendarDate.isAfter(_max)) {
_calendarDate = new CalendarDate(_max);
}
}
// Re-render with updated values
renderCalendar({
type: "date",
value: _calendarDate || new CalendarDate(),
});
You can also share your feedback on Copilot code review. Take the survey.
|
|
||
| <hr /> | ||
| <h2>DatePicker — Event-based (min/max = today, Date object)</h2> | ||
| <goab-form-item label="Event-based DatePicker"> |
e0a4607 to
ebb8b81
Compare
| minString(): string | undefined { | ||
| if (!this.min) return undefined; | ||
| return formatDate(this.min); | ||
| } | ||
|
|
||
| maxString(): string | undefined { | ||
| if (!this.max) return undefined; | ||
| return formatDate(this.max); | ||
| } |
There was a problem hiding this comment.
The Date type should show a warning in the console, since React only accepts strings.
|
|
||
| if (val instanceof Date) { | ||
| return val.toISOString(); | ||
| return formatDate(val); |
There was a problem hiding this comment.
The console warning is needed here as well for the min/max dates that should be strings.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 27 changed files in this pull request and generated 10 comments.
You can also share your feedback on Copilot code review. Take the survey.
| resolve: { | ||
| alias: { | ||
| "@abgov/ui-components-common": path.resolve( | ||
| __dirname, | ||
| "../../node_modules/@abgov/ui-components-common", |
| `goa-calendar ${name ?? testid}: Invalid min date provided: ${min}.`, | ||
| ); | ||
| } | ||
|
|
||
| _max = max ? new CalendarDate(max) : new CalendarDate().addYears(+5); | ||
| if (!_max.isValid()) { | ||
| console.error( | ||
| `goa-calendar ${name ?? testid}: Invalid max date provided: ${max}.`, | ||
| ); |
There was a problem hiding this comment.
This is a good catch, but I don't want to set a value in case of an error. In a case like this I would rather the control act broken than to silently allow invalid input.
| include: [ | ||
| "libs/web-components/src/**/*.{test,spec}.ts", | ||
| ], | ||
| include: ["libs/web-components/src/**/*.{test,spec}.ts"], |
| if (!_max.isValid()) { | ||
| console.error( | ||
| `goa-calendar ${name ?? testid}: Invalid max date provided: ${max}.`, | ||
| ); |
There was a problem hiding this comment.
As with the comment above, I don't want to set a value in the case of an invalid input. In a case like this I would rather the control act broken than to silently allow invalid input.
There was a problem hiding this comment.
I think that the suggested code would be a good thing to have, I don't like the fact that this logic has to be done every single time. Maybe the following code would be a little cleaner..
constructor(value?: CalendarDateInput, fallback?: CalendarDate) {
if (value || value === 0) {
this._dateNums = CalendarDate.parse(value);
if (!this.isValid()) {
if (fallback) {
this._dateNums = [fallback.year, fallback.month, fallback.day];
} else {
this._dateNums = [0, 0, 0];
}
console.error(
`The CalendarDate value of ${value}, is invalid. Falling back to ${this.date}`,
);
}
} else {
this._dateNums = CalendarDate.parse(new Date());
}
}This would then allow for
_min = new CalendarDate(min, new CalendarDate().addYears(-5));It is a little weird in using a CalendarDate as a fallback for the CalendarDate constructor, but it does clean things up a bit.
What do you think?
There was a problem hiding this comment.
Unresolving this comment for discussion.
There was a problem hiding this comment.
@chrisolsen A default might be okay for value, but I don't think so for min/max.. The main problem I see is that the programmer is specifying a min/max range for value (even though it is somehow invalid), and by falling back to a default we could end up letting bad data slip through because we used a fallback without a warning and they couldn't handle it client-side..
I would also suggest we don't provide fallbacks in the constructor, but rather to the parse method. If the constructor gets invalid input its usually better to raise an exception. Parsing is a situation where the possibility of invalid inputs might be more common. Using a non-ctor method would also allow us to return a non-CalendarDate object like null or undefined.
There was a problem hiding this comment.
Parse is a private method so the logic could be moved into it, but the constructor still has to accept it.
The parse method returns an array of numbers which is something not that useful to be used externally, so there is no way of using it.
cd46d8e to
27ca9b5
Compare
vanessatran-ddi
left a comment
There was a problem hiding this comment.
I have only 1 comment, I tested and everything works perfectly.
| private minWarningLogged = false; | ||
| private maxWarningLogged = false; | ||
|
|
||
| valueString(): string | undefined { |
There was a problem hiding this comment.
@vanessatran-ddi I thought about this but couldn't decide. I'm glad you suggested this.
There was a problem hiding this comment.
I think this could be cleaned up even more with something like
// This class would exist within the common lib (name of the class and methods are not set in stone)
class DoOnce {
private done: Set<string>;
private constructor() {
this.done = new Set();
}
do(id: string, fn): string {
if (!this.done.has(id)) {
this.done.add(id);
fn();
}
}
}
// declared within the angular component
const once = new DoOnce();
// angular compoent functions
minString(val: Date | string | null): string {
if (val instanceof Date) {
once.do("min", () => console.warn("ya, that's deprecated"));
}
return new CalendarDate(val).toString();
}
maxString(val: Date | string | null): string {
if (val instanceof Date) {
once.do("max", () => console.warn("ya, that's deprecated"));
}
return new CalendarDate(val).toString();
}
There was a problem hiding this comment.
@chrisolsen it seemed like something that might be useful in the polyfills, too, but honestly it doesn't feel like a huge win. After Prettier doing what Prettier does it onto multiple lines and looks like this:
valueString(): string | undefined {
if (this.value instanceof Date) {
this.once.do("value", () => {
console.warn(
`GoabxCalendar: Using Date for 'value' is deprecated. Use a string in YYYY-MM-DD format instead.`,
);
});
}
return this.formatValue("value", this.value);
}I tried to cut out the if block to remove some indentation and it looks like this:
valueString(): string | undefined {
this.once.when(this.value instanceof Date).do("value", () => {
console.warn(
`GoabxCalendar: Using Date for 'value' is deprecated. Use a string in YYYY-MM-DD format instead.`,
);
});
return this.formatValue("value", this.value);
}I'm not sure its a huge win, but if you like it I can check it in.
The Once class looks like this:
export class Once {
private done: Set<string>;
constructor() {
this.done = new Set();
}
when(condition: boolean): Once {
if (condition) {
return this;
} else {
return new DoNothing();
}
}
do(id: string, fn: () => void): Once {
if (!this.done.has(id)) {
this.done.add(id);
fn();
}
return this;
}
}
class DoNothing extends Once {
override do(id: string, fn: () => void): Once {
return this;
}
}There was a problem hiding this comment.
I like your version better.
The thing is that right now this pattern is being done 4 times, and each time you have to create a Set and add the logic. I can also see the Once being useful in other scenarios as well.
27ca9b5 to
a243a64
Compare
789a0a1 to
ba3ad0d
Compare
…pass Dates to the Svelte Calendar component - make CalendarDate global and use it for date formatting - fix date handling in React components - add Once library to execute code only once - normalize string values at the formatValue level for value, min, max
ba3ad0d to
a9c5482
Compare
(From Copilot) The Vitest alias for "@abgov/ui-components-common" points at "./dist/libs/common/index.js". This will fail (or test stale code) when running tests without a prior build of the common library. Prefer resolving to the source entry via tsconfig paths ("libs/common/src/index.ts") or ensure the test command builds common before Vitest runs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
🎉 This PR is included in version 2.0.0-next.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.0.0-next.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 7.0.0-next.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 5.0.0-next.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 7.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 5.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |


Before (the change)
The Calendar wasn't showing years if the min or max were equal to Today.
There was a problem with data types, parsing and passing strings and JavaScript Dates from Angular components.
After (the change)
This passes the Dates as ISO strings from Angular to web-component. ISO date strings are more easily parsed by the CalendarDate.parse() method.
Make sure that you've checked the boxes below before you submit the PR
Steps needed to test