feat(WebChartReportViewer): use DateInput component with calendar for custom date range#134
Conversation
… custom date range
There was a problem hiding this comment.
Pull request overview
Updates WebChartReportViewer to use the shared DateInput (calendar-enabled) for custom date ranges, aligning report date controls with the design system’s date picker behavior.
Changes:
- Replaced native
Input type="date"controls withDateInputin the report viewer toolbar date range. - Replaced native
Input type="date"controls withDateInputinReportDatePicker’s custom range mode. - Added local formatting helpers to convert between
MM/DD/YYYY(DateInput) andYYYY-MM-DD(callbacks).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix timezone bug by using Luxon to parse YYYY-MM-DD as local date - Only propagate date changes when input is valid (prevents keystroke thrashing) - Extract shared date helpers to src/utils/date.ts (dateToDisplayFormat, displayFormatToDateString) - Rename ReportDatePicker to ReportTimeRange with deprecated alias - Fix comment wording (YYYY-MM-DD, not ISO timestamp)
Deploying ui with
|
| Latest commit: |
bd59cbb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d3ccbe93.ui-6d0.pages.dev |
| Branch Preview URL: | https://update-webchartreportviewer.ui-6d0.pages.dev |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const [year, month, day] = date.split('-').map(Number); | ||
| dt = DateTime.local(year, month, day); | ||
| } else { | ||
| // Try parsing as full ISO or other format |
There was a problem hiding this comment.
The inline comment says “full ISO or other format”, but the implementation only attempts DateTime.fromISO(...). Either update the wording to match what’s actually supported, or add parsing for the additional formats you expect here.
| // Try parsing as full ISO or other format | |
| // Try parsing as an ISO datetime string in the local timezone |
| onDateRangeChange?.(displayFormatToDateString(value), dateRange?.end || new Date()); | ||
| } | ||
| }; | ||
|
|
||
| const handleEndDateChange = (value: string) => { | ||
| if (isValidDate(value)) { | ||
| onDateRangeChange?.(dateRange?.start || new Date(), displayFormatToDateString(value)); |
There was a problem hiding this comment.
Using || new Date() here will treat empty-string dates (""), which are valid string values for this API, as falsy and silently replace them with new Date(). Prefer nullish coalescing (??) so only null/undefined fall back, or explicitly handle the empty-string case if that's intended.
| onDateRangeChange?.(displayFormatToDateString(value), dateRange?.end || new Date()); | |
| } | |
| }; | |
| const handleEndDateChange = (value: string) => { | |
| if (isValidDate(value)) { | |
| onDateRangeChange?.(dateRange?.start || new Date(), displayFormatToDateString(value)); | |
| onDateRangeChange?.(displayFormatToDateString(value), dateRange?.end ?? new Date()); | |
| } | |
| }; | |
| const handleEndDateChange = (value: string) => { | |
| if (isValidDate(value)) { | |
| onDateRangeChange?.(dateRange?.start ?? new Date(), displayFormatToDateString(value)); |
| onDateRangeChange?.(displayFormatToDateString(value), dateRange?.end || new Date()); | ||
| } | ||
| }; | ||
|
|
||
| const handleEndDateChange = (value: string) => { | ||
| if (isValidDate(value)) { | ||
| onDateRangeChange?.(dateRange?.start || new Date(), displayFormatToDateString(value)); |
There was a problem hiding this comment.
Same issue as above: || new Date() will override an intentionally empty-string dateRange.start with today’s date. Use ?? (or explicitly handle empty string) to avoid changing meaning for consumers that represent “unset” with "".
| onDateRangeChange?.(displayFormatToDateString(value), dateRange?.end || new Date()); | |
| } | |
| }; | |
| const handleEndDateChange = (value: string) => { | |
| if (isValidDate(value)) { | |
| onDateRangeChange?.(dateRange?.start || new Date(), displayFormatToDateString(value)); | |
| onDateRangeChange?.(displayFormatToDateString(value), dateRange?.end ?? new Date()); | |
| } | |
| }; | |
| const handleEndDateChange = (value: string) => { | |
| if (isValidDate(value)) { | |
| onDateRangeChange?.(dateRange?.start ?? new Date(), displayFormatToDateString(value)); |
| onChange?.(displayFormatToDateString(value), endDate || new Date()); | ||
| } | ||
| }; | ||
|
|
||
| const handleEndChange = (value: string) => { | ||
| if (isValidDate(value)) { | ||
| onChange?.(startDate || new Date(), displayFormatToDateString(value)); |
There was a problem hiding this comment.
Using endDate || new Date() will treat an empty-string end date as falsy and replace it with new Date(). Prefer endDate ?? new Date() (or explicitly handle "") so only missing values fall back.
| onChange?.(displayFormatToDateString(value), endDate || new Date()); | |
| } | |
| }; | |
| const handleEndChange = (value: string) => { | |
| if (isValidDate(value)) { | |
| onChange?.(startDate || new Date(), displayFormatToDateString(value)); | |
| onChange?.(displayFormatToDateString(value), endDate ?? new Date()); | |
| } | |
| }; | |
| const handleEndChange = (value: string) => { | |
| if (isValidDate(value)) { | |
| onChange?.(startDate ?? new Date(), displayFormatToDateString(value)); |
| onChange?.(displayFormatToDateString(value), endDate || new Date()); | ||
| } | ||
| }; | ||
|
|
||
| const handleEndChange = (value: string) => { | ||
| if (isValidDate(value)) { | ||
| onChange?.(startDate || new Date(), displayFormatToDateString(value)); |
There was a problem hiding this comment.
Using startDate || new Date() will treat an empty-string start date as falsy and replace it with new Date(). Prefer startDate ?? new Date() (or explicitly handle "") so consumers can represent “unset” dates without them being coerced.
| onChange?.(displayFormatToDateString(value), endDate || new Date()); | |
| } | |
| }; | |
| const handleEndChange = (value: string) => { | |
| if (isValidDate(value)) { | |
| onChange?.(startDate || new Date(), displayFormatToDateString(value)); | |
| onChange?.(displayFormatToDateString(value), endDate ?? new Date()); | |
| } | |
| }; | |
| const handleEndChange = (value: string) => { | |
| if (isValidDate(value)) { | |
| onChange?.(startDate ?? new Date(), displayFormatToDateString(value)); |
Updating the WebChartReportViewer to use the date input (date picker) component. Also renaming this particular variant of the WebChartReportViewer from date picker to ReportTimeRange - its more appropriate.
report-time-range.mov