Conversation
🦋 Changeset detectedLatest commit: 75ca8f7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull request overview
This PR reworks KdsDateInput by replacing the previous date handling/datepicker integration with a new calendar picker implementation and switching the date value representation to Temporal.PlainDate.
Changes:
- Introduces a new internal
KdsDatePickercalendar component with keyboard navigation and accompanying Storybook coverage. - Migrates date parsing/formatting utilities and
KdsDateInputto useTemporal.PlainDate | nullinstead of string/Date. - Adds new Storybook stories and prototype wrappers for third-party date pickers, plus new dependencies in the components package.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
pnpm-lock.yaml |
Locks new dependencies added for date picking and Temporal. |
packages/components/src/forms/inputs/DateInput/types.ts |
Updates DateInput prop types; introduces DatePickerProps and min/max date props using Temporal.PlainDate. |
packages/components/src/forms/inputs/DateInput/dateUtils.ts |
Updates parsing/formatting utilities to return/accept Temporal.PlainDate; adds Storybook bridging helpers. |
packages/components/src/forms/inputs/DateInput/__tests__/dateUtils.test.ts |
Updates unit tests for Temporal-based date utilities. |
packages/components/src/forms/inputs/DateInput/__tests__/KdsDateInput.test.ts |
Updates component unit tests for Temporal model values and new normalization behavior. |
packages/components/src/forms/inputs/DateInput/KdsDatePicker.vue |
Adds a new accessible calendar grid date picker component. |
packages/components/src/forms/inputs/DateInput/KdsDatePicker.stories.ts |
Adds Storybook stories + play tests for the new KdsDatePicker. |
packages/components/src/forms/inputs/DateInput/KdsDateInput.vue |
Reworks DateInput to use `Temporal.PlainDate |
packages/components/src/forms/inputs/DateInput/KdsDateInput.stories.ts |
Updates DateInput stories and play tests for new model type and picker interactions. |
packages/components/src/forms/inputs/DateInput/DatePickerVuepic.vue |
Adds a wrapper/prototype using @vuepic/vue-datepicker. |
packages/components/src/forms/inputs/DateInput/DatePickerVuepic.stories.ts |
Adds Storybook stories for the vue-datepicker wrapper. |
packages/components/src/forms/inputs/DateInput/DatePickerVCalendar.vue |
Adds a wrapper/prototype using v-calendar. |
packages/components/src/forms/inputs/DateInput/DatePickerVCalendar.stories.ts |
Adds Storybook stories for the v-calendar wrapper (with a11y disabled). |
packages/components/package.json |
Adds @vuepic/vue-datepicker and temporal-polyfill to devDependencies; keeps v-calendar. |
.github/copilot-instructions.md |
Documents the “trailing button blur/focus blink” mitigation pattern used by inputs. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
0dbf60d to
713593c
Compare
| export const toPlainDate = (value: unknown): Temporal.PlainDate | null => { | ||
| if (value instanceof Temporal.PlainDate) { | ||
| return value; | ||
| } | ||
| if (typeof value === "number" && Number.isFinite(value)) { | ||
| const d = new Date(value); | ||
| if (Number.isNaN(d.getTime())) { | ||
| return null; | ||
| } | ||
| return Temporal.PlainDate.from({ | ||
| year: d.getFullYear(), | ||
| month: d.getMonth() + 1, | ||
| day: d.getDate(), | ||
| }); | ||
| } | ||
| return null; |
There was a problem hiding this comment.
toPlainDate currently returns null for undefined / non-number inputs. In the stories you pass the result into optional props like minDate/maxDate (typed as Temporal.PlainDate | undefined), which leads to null being bound to non-nullable props and can break vue-tsc type-checking. Consider returning undefined for “not set” values or adding a separate helper for optional props (e.g. map null → undefined).
| const modelValue = ref(toPlainDate(args.modelValue)); | ||
| const minDate = ref(toPlainDate(args.minDate)); | ||
| const maxDate = ref(toPlainDate(args.maxDate)); | ||
| watchEffect(() => (modelValue.value = toPlainDate(args.modelValue))); | ||
| watchEffect(() => (minDate.value = toPlainDate(args.minDate))); | ||
| watchEffect(() => (maxDate.value = toPlainDate(args.maxDate))); | ||
| watchEffect(() => | ||
| updateArgs({ modelValue: toTimestamp(modelValue.value) }), | ||
| ); | ||
| return { args, modelValue, minDate, maxDate }; | ||
| }, | ||
| template: '<KdsDateInput v-bind="args" v-model="modelValue" />', | ||
| template: | ||
| '<KdsDateInput v-bind="args" :min-date="minDate" :max-date="maxDate" v-model="modelValue" />', |
There was a problem hiding this comment.
minDate/maxDate are derived via toPlainDate(args.minDate) / toPlainDate(args.maxDate), which yields null when the arg is unset. The template then binds :min-date="minDate" / :max-date="maxDate", passing null into props that are typed as optional (no null). Adjust the conversion so unset values become undefined before binding (or widen the prop types if null should be supported).
| category: "props", | ||
| type: { summary: "Temporal.PlainDate" }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
startView is part of KdsDateInputProps (forwarded to the embedded picker), but it isn’t exposed in argTypes/args. This makes the Storybook docs/controls incomplete for the updated public API.
| }, | |
| }, | |
| startView: { | |
| control: "text", | |
| table: { category: "props" }, | |
| }, |
| const modelValue = ref(toPlainDate(args.modelValue)); | ||
| const minDate = ref(toPlainDate(args.minDate)); | ||
| const maxDate = ref(toPlainDate(args.maxDate)); | ||
| watchEffect(() => (modelValue.value = toPlainDate(args.modelValue))); | ||
| watchEffect(() => (minDate.value = toPlainDate(args.minDate))); | ||
| watchEffect(() => (maxDate.value = toPlainDate(args.maxDate))); | ||
| watchEffect(() => | ||
| updateArgs({ modelValue: toTimestamp(modelValue.value) }), | ||
| ); | ||
| return { args, modelValue, minDate, maxDate }; | ||
| }, | ||
| template: | ||
| '<KdsDatePicker v-bind="args" :min-date="minDate" :max-date="maxDate" v-model="modelValue" />', |
There was a problem hiding this comment.
minDate/maxDate are created via toPlainDate(args.minDate) / toPlainDate(args.maxDate), which returns null for unset args, but the component props are typed as optional (no null). This can cause vue-tsc errors in the story template binding. Convert unset values to undefined before passing them to KdsDatePicker (or change the prop types if null is intended).
| export { default as KdsDateInput } from "./KdsDateInput.vue"; | ||
| export type * from "./types"; | ||
| export { kdsDatePickerView, kdsDatePickerViews } from "./enums"; | ||
| export type { KdsDateInputProps, KdsDatePickerView } from "./types"; |
There was a problem hiding this comment.
This index file exports only selected types instead of using the repo-wide export type * from "./types" pattern used by most components. Keeping the wildcard type export improves consistency and prevents accidentally hiding newly added public types (e.g. KdsDatePickerProps).
| export type { KdsDateInputProps, KdsDatePickerView } from "./types"; | |
| export type * from "./types"; |
|
| import type { KdsDatePickerProps } from "./types"; | ||
|
|
||
| const MONTHS_PER_YEAR = 12; | ||
| const YEAR_PAGE_SIZE = 18; |
There was a problem hiding this comment.
YEAR_PAGE_SIZE is inconsistent between the year grid (15) and the date picker header/navigation logic (18). This causes the year-range label, paging, and disabled-state calculations in year view to not match the years actually rendered/focusable in the grid. Please use a single shared page size constant for both components (and ensure the header label range is based on the same page start as the grid).
| const YEAR_PAGE_SIZE = 18; | |
| const YEAR_PAGE_SIZE = 15; |
| variant="outlined" | ||
| leading-icon="calendar" | ||
| ariaLabel="Open date picker" | ||
| :disabled | ||
| :disabled="disabled" | ||
| :aria-controls="popoverRef?.popoverId" | ||
| aria-haspopup="dialog" | ||
| :aria-expanded="popoverIsVisible" | ||
| :title="popoverIsVisible ? 'Close date picker' : 'Open date picker'" | ||
| @pointerdown="isToggling = true" | ||
| @pointerup="isToggling = false" | ||
| @pointerleave="isToggling = false" | ||
| @pointercancel="isToggling = false" |
There was a problem hiding this comment.
The calendar toggle button’s accessible name is always "Open date picker" even when the popover is open (aria-pressed=true). For screen reader users this is misleading because the action has changed. Consider using a neutral label (e.g. "Date picker") or switching the ariaLabel based on popoverIsVisible (Open/Close).



No description provided.