Repository navigation
feat(DateRangeInput): show the range in the calendar - #9653
nynexman4464 wants to merge 5 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision, and three changes are needed whatever that decision is.
The behavior rests on spec:AST-111 FR15–FR17 and DEC-7, which are only in #9605. Overriding an explicit presentation="native" in non-Gregorian calendars contradicts current spec:AST-043 FR1–FR3 and architecture:public-component-api INV16, so an engineering owner needs to approve that direction, the new DateRangeInput calendar prop, and the locale-digit day numbers in Gregorian bottom sheets. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner.
Blocking findings:
- The PR changes 2,479 authored lines; please split it along its existing commits (the native-surface override, the bottom sheet in calendar months, and the
DateRangeInputcalendarprop) so each PR stays under 2,000 lines. spec:AST-043FR1 and FR3 andcomponent:DateTimeInputFR3 still mapnativeandalwaysto the native surface; add the calendar exception to them as well as to AST-043 FR2.- In the Japanese calendar, the bottom sheet's year wheel labels 2019 as 令和元 while January–April 2019 are 平成31; label an era-change year with both eras, or from the month on screen.
[Automated review]
| always show the native surface, with no eligibility fallbacks — a state the | ||
| native control cannot express (seconds, non-default step/increment, preset | ||
| time options, no usable native control) is the caller's explicit choice. | ||
| The one exception is the calendar: with a calendar other than `gregory` or |
There was a problem hiding this comment.
This exception lands only in FR2. FR1's table (line 62) still maps native to Native on both pointers, and FR3 (line 111) still says "always maps to the native surface on every pointer". component:DateTimeInput FR3 also still says presentation "MUST follow spec:AST-043 FR1–FR4". Under a non-Gregorian calendar the code sends all three to the popover or sheet, so please state the calendar exception in FR1 and FR3, and in DateTimeInput FR3, too.
| return String(year); | ||
| } | ||
| const months = calendarMonthsInCalendarYear(calendar, year); | ||
| const middle = calendarMonthOfYear( |
There was a problem hiding this comment.
For Japanese, the wheel's years are Gregorian years labelled from their middle month, so 2019 reads 令和元. The panes and title for January–April 2019 read 平成31年1月–平成31年4月, and 1912 has the same split. Could an era-change year carry both era years (平成31・令和元), or take the selected year's label from the month the sheet shows? A 2019 case in "labels Japanese years with their era" would pin it.
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision, and two changes are needed whatever that decision is.
This PR intentionally changes established spec. Overriding an explicit presentation="native" in non-Gregorian calendars contradicts current spec:AST-043 FR1–FR3 and architecture:public-component-api INV16, and the calendar bottom sheet, the locale-digit day numbers in Gregorian sheets and the new DateRangeInput calendar prop rest on spec:AST-111 FR15–FR17, which are only in #9605. An engineering owner needs to approve that direction, which updates those existing claims, by commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner.
Blocking findings:
- The PR changes 2,551 authored lines; please split it into the native-surface override, the calendar bottom sheet, and the
DateRangeInputcalendarprop so each PR stays under 2,000 lines. - Proposed
spec:AST-043FR1–FR3 still mapnative,adaptive-nativeandalwaysto the native surface, andcomponent:DateTimeInputFR3 still points at them; state the calendar exception in those clauses too, not only for an explicitnativein FR2.
The Japanese era label in era-change years now names both eras, as asked.
[Automated review]
| | ---- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------- | | ||
| | FR1 | DateTimeInput MUST reflect one controlled date-time value across its visible segments and active picker, and MUST NOT reveal a competing selected date after the parent accepts a clear. | Public docs, controlled component seam, current source, focused tests | | ||
| | FR2 | Every committed date or time change MUST emit one valid combined ISO date-time value; clearing MUST emit `undefined`; changing time while no date is committed MUST emit nothing. | Public types, docs, current source, focused tests | | ||
| | FR3 | `presentation`, deprecated `nativePicker`, precedence, pointer adaptation, forced surfaces, and per-segment native fallbacks MUST follow `spec:AST-043` FR1–FR4. The touch date sheet's month heading and month-pane names follow `component:DateInput` FR6. | `spec:AST-043` | |
There was a problem hiding this comment.
FR3 still says presentation "MUST follow spec:AST-043 FR1–FR4", and AST-043 FR1–FR3 still map native, adaptive-native and always to the native surface. FR11 below sends all of them to the popover or the bottom sheet outside gregory and iso8601. Could FR3 name the FR11 exception (and AST-043 FR1–FR3 carry it too), so the records agree with the code?
| return <TouchDateField {...rest} isDisabled={isDisabled} />; | ||
| return ( | ||
| <TouchDateField | ||
| // Its month indexes belong to the calendar. |
There was a problem hiding this comment.
Non-blocking (comment style): "Its" has no clear subject here. Suggested: // Month indexes depend on the calendar. A new calendar remounts the sheet.
| return <TouchDateTimeField {...sheetProps} />; | ||
| } | ||
| case 'sheet': | ||
| // Its month indexes belong to the calendar. |
There was a problem hiding this comment.
Non-blocking (comment style): same wording as in DateInput. Suggested: // Month indexes depend on the calendar. A new calendar remounts the sheet.
7bf40f8 to
64eb7d5
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision, and two changes are needed whatever that decision is.
This PR intentionally changes established spec: overriding an explicit presentation="native" in non-Gregorian calendars contradicts current spec:AST-043 FR1–FR3 and architecture:public-component-api INV16, and the calendar bottom sheet (including its era-label rule), the time segment leaving the native control, and the new DateRangeInput calendar prop rest on spec:AST-111 FR15–FR17, which is still a draft in #9605. An engineering owner needs to approve that direction, which updates those existing claims, by commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner.
Blocking findings:
- The PR changes 2,865 authored lines; please split it into the native-surface override, the calendar bottom sheet, and the
DateRangeInputcalendarprop so each PR stays under 2,000 lines. - The proposed
component:DateRangeInputFR5 reuses the id thatmainalready gives the preset reflow requirement, so after a rebase the record would hold two FR5 rows; please give the calendar requirement the next free id in the record, its verification row and thecomponent:Calendarrelationship line.
The AST-043 FR1, FR3 and DateTimeInput FR3 wording now carries the calendar exception, as asked earlier.
[Automated review]
| | FR2 | When presets are present, each preset remains an independent button in one labeled group; the applied preset reflects current state and a preset whose endpoint violates `min`, `max`, or `dateConstraints`, or whose range violates `minRangeSpan` or `maxRangeSpan`, reflects disabled state. | Current source, accessibility comments, and focused tests | Verified current behavior | | ||
| | FR3 | The preset group and each preset button expose stable theme targets; selected and disabled are states of the preset-button target rather than separate targets. | `architecture:component-theming-surface`; consumer demand for preset theming | Approved additive contract | | ||
| | FR4 | Adding theme targets MUST NOT change the Popover, Calendar, button, focus, or selection semantics those elements already own. | Composition boundary and compatibility goal | Approved additive contract | | ||
| | FR5 | The trigger text and the popover Calendar show the resolved calendar (`spec:AST-111` FR2, FR17): the trigger writes each end of the range in the calendar, with the year when either end falls outside the current calendar year, and the Calendar lays out calendar months. The field has no typed entry. `value`, `onChange`, `min`, `max`, presets, and span bounds stay Gregorian ISO dates (`spec:AST-111` FR3). | `spec:AST-111` FR3, FR17; current source and calendar tests | Verified current behavior | |
There was a problem hiding this comment.
main already uses FR5 in this record for the preset reflow requirement ("the preset group stacks above the calendar"), added after this stack branched. After a rebase this table would hold two FR5 rows; could the calendar requirement take the next free id, with its verification row and the component:Calendar relationship line updated to match?
| ); | ||
| const monthYearLabel = plainDateFormatStandaloneMonthYear( | ||
| {year, month, day: 1}, | ||
| // The day the picker keeps: the selected date's day of its month. The month |
There was a problem hiding this comment.
Non-blocking (comment style): the first sentence restates calendarDayOfMonth(calendar, selectedDate) and the second runs 29 words. Suggested: // In a month that spans an era change, the heading takes its era from this day, as the year wheel does. The same comment is in TouchDateTimeField.tsx.
| return result; | ||
| }, [monthModel, calendar, locale, minMonthIndex, maxMonthIndex]); | ||
|
|
||
| // A year that spans an era change names the era of the date the wheels |
There was a problem hiding this comment.
Non-blocking (comment style): this sentence runs 30 words. Suggested: // A year that spans an era change takes the era of the shown date, as on iOS. Its row text changes as the month turns.
90a16cd to
79e1235
Compare
64eb7d5 to
a4b6045
Compare
nynexman4464
changed the title
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision, and two fixes are needed whatever that decision is.
DateRangeInput's new calendar prop, and its trigger text and popover following the application's calendar, rest on spec:AST-111 FR17, which is still a draft in #9605. The change also departs from current architecture:internationalization INV10, which keeps date-component grids Gregorian. An engineering owner needs to approve that direction, including the proposed component:DateRangeInput FR5, its calendar concept row and its spec:AST-013 FR12 exception, by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner.
Blocking findings:
- The proposed
component:DateRangeInputFR5 reuses the idmainalready gives the preset reflow requirement, so please give the calendar requirement the next free id and update its verification row, its relationship line and the citation incomponent:Calendar. - The
calendarprop's JSDoc inInternationalizationProvider.tsx(lines 111–113) still saysDateRangeInputshows Gregorian dates, so please listDateRangeInputthere and in theInternationalizationContextValuecomment, as the doc module and the record now do.
[Automated review]
| required: false, | ||
| description: | ||
| "Calendar system that `Calendar`, `DateInput`, and `DateTimeInput` in the subtree show, such as 'persian', 'hebrew', or 'buddhist'. `DateRangeInput` shows Gregorian dates. When omitted, a `-u-ca-` keyword in `locale` selects the calendar, else Gregorian; a locale's default calendar never applies. A component's own `calendar` prop takes precedence. Values stay Gregorian ISO dates. Calendars other than gregory, iso8601, buddhist, roc, and japanese need `Temporal`; where the runtime lacks it, load a polyfill such as `temporal-polyfill/full/global` on the server and in the browser, or those calendars show Gregorian dates and warn in development.", | ||
| "Calendar system that `Calendar`, `DateInput`, `DateRangeInput`, and `DateTimeInput` in the subtree show, such as 'persian', 'hebrew', or 'buddhist'. When omitted, a `-u-ca-` keyword in `locale` selects the calendar, else Gregorian; a locale's default calendar never applies. A component's own `calendar` prop takes precedence. Values stay Gregorian ISO dates. Calendars other than gregory, iso8601, buddhist, roc, and japanese need `Temporal`; where the runtime lacks it, load a polyfill such as `temporal-polyfill/full/global` on the server and in the browser, or those calendars show Gregorian dates and warn in development.", |
There was a problem hiding this comment.
This description now lists DateRangeInput, but the prop's JSDoc in InternationalizationProvider.tsx (lines 111–113) still says "DateRangeInput shows Gregorian dates", and the InternationalizationContextValue.calendar comment in InternationalizationContext.ts (lines 34–35) leaves it out. Could both say the same as this line, so the hover documentation matches what ships?
6afec54 to
264269c
Compare
a5536ba to
35aa2c3
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision, and two fixes are needed whatever that decision is.
DateRangeInput's new calendar prop, and its trigger text and popover following the application's calendar, rest on spec:AST-111 FR17, which is still a draft in #9605, and depart from current architecture:internationalization INV10, which keeps date grids Gregorian. An engineering owner needs to approve that governing spec for this addition, including the proposed component:DateRangeInput FR5, its calendar concept row and its spec:AST-013 FR12 exception, by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner.
Blocking findings:
- The proposed
component:DateRangeInputFR5 reuses the idmainalready gives the preset reflow requirement, so please give the calendar requirement the next free id and update its verification row, its relationship line and the citation incomponent:Calendar. - The
calendarprop's JSDoc inInternationalizationProvider.tsx(lines 111–113) still saysDateRangeInputshows Gregorian dates, so please listDateRangeInputthere and in theInternationalizationContextValuecomment, as the doc module and the record now do.
[Automated review]
264269c to
6361f62
Compare
35aa2c3 to
ce263ee
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision, and two fixes are needed whatever that decision is.
DateRangeInput's new calendar prop, and its trigger text and popover following the application's calendar, rest on spec:AST-111 FR17, which is still a draft in #9605, and depart from current architecture:internationalization INV10, which keeps date grids Gregorian. An engineering owner needs to approve that governing spec for this addition, including the proposed component:DateRangeInput FR5, its calendar concept row and its spec:AST-013 FR12 exception, by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner.
Blocking findings:
- The proposed
component:DateRangeInputFR5 reuses the idmainalready gives the preset reflow requirement, so please give the calendar requirement the next free id and update its verification row, its relationship line and the citation incomponent:Calendar. - The
calendarprop's JSDoc inInternationalizationProvider.tsx(lines 111–113) still saysDateRangeInputshows Gregorian dates, so please listDateRangeInputthere and in theInternationalizationContextValuecomment, as the doc module and the record now do.
[Automated review]
6361f62 to
9662010
Compare
ce263ee to
cb8dd2f
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision; no other changes are needed.
DateRangeInput's new calendar prop, its calendar-system trigger text and popover, and the -u-ca- locale keyword now reaching the range text rest on spec:AST-111 FR17 and DEC-2, which are still a draft in #9605, and depart from the current architecture:internationalization INV10, which keeps date grids Gregorian and stops a locale from selecting another calendar. The decision has to settle:
- whether
DateRangeInputfollows the application's calendar throughcalendar?: CalendarSystem, defaulting to the provider's calendar, else'gregory'; - whether a
-u-ca-keyword changing released Gregorian range text is nonbreaking, as the proposed record states, or needs aspec:AST-017compatibility class; - the proposed
component:DateRangeInputFR6, itscalendarconcept row andTemporalexception, and the matchingcomponent:InternationalizationProviderandcomponent:Calendarrecord lines.
An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner.
[Automated review]
9662010 to
6b4a555
Compare
cb8dd2f to
3cef6a3
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision; no other changes are needed.
DateRangeInput's new calendar prop, its calendar-system trigger text and popover, and the -u-ca- locale keyword now reaching the range text rest on spec:AST-111 FR17 and DEC-2, which are still a draft in #9605, and they depart from current architecture:internationalization INV10, which keeps date grids Gregorian and stops a locale from selecting another calendar for date components. What an engineering owner needs to approve or reject:
- the public
calendar?: CalendarSystemprop onDateRangeInput, defaulting to theInternationalizationProvidercalendar, else'gregory'; - the trigger text, its accessible name and the popover Calendar shown in that calendar, with the year when either end leaves the current calendar year;
- a provider locale with an explicit
-u-ca-keyword, such asth-TH-u-ca-buddhist, changing range text that the latest release shows in Gregorian, recorded as nonbreaking; - the proposed record text:
component:DateRangeInputFR6, itscalendarconcept row andspec:AST-013FR12 exception,component:InternationalizationProviderFR10 and thecomponent:Calendarrelationship line.
An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner. Two optional follow-ups: FR6 could name the trigger's accessible name with a test that asserts it, and a th-TH-u-ca-buddhist test would pin the locale-keyword behavior once it is decided.
[Automated review]
| | FR3 | The preset group and each preset button expose stable theme targets; selected and disabled are states of the preset-button target rather than separate targets. | `architecture:component-theming-surface`; consumer demand for preset theming | Approved additive contract | | ||
| | FR4 | Adding theme targets MUST NOT change the Popover, Calendar, button, focus, or selection semantics those elements already own. | Composition boundary and compatibility goal | Approved additive contract | | ||
| | FR5 | When the preset group and a one-month calendar do not fit side by side in the layer, the preset group stacks above the calendar, and every month grid and the month navigation stay inside the layer (`spec:AST-020` FR1, WCAG 2.2 SC 1.4.10). Visual order matches DOM order. When they fit, the presets stay beside the calendar. When the content is taller than the layer's viewport cap, it scrolls inside the layer. | `spec:AST-020` FR1; 390 px, 390 × 664 px and 440 px reflow checks | Verified current behavior | | ||
| | FR6 | The trigger text and the popover Calendar show the resolved calendar (`spec:AST-111` FR2, FR17): the trigger writes each end of the range in the calendar, with the year when either end falls outside the current calendar year, and the Calendar lays out calendar months. The field has no typed entry. `value`, `onChange`, `min`, `max`, presets, and span bounds stay Gregorian ISO dates (`spec:AST-111` FR3). | `spec:AST-111` FR3, FR17; current source and calendar tests | Verified current behavior | |
There was a problem hiding this comment.
Non-blocking: the trigger's accessible name is built from the label and this text (DateRangeInput.tsx:744-746), so screen-reader users hear Stay: Aban 8 – Aban 11. Could FR6 say "the trigger text and its accessible name", with one test that asserts the name?
6b4a555 to
93bca81
Compare
3cef6a3 to
a26bf47
Compare
DateRangeInput takes a calendar prop and otherwise follows the provider's calendar. Its trigger text and popover calendar use the calendar; values stay Gregorian ISO dates.
…d the -u-ca- request The provider's calendar prop docs and record list DateRangeInput. The DateRangeInput record and changeset state that an explicit -u-ca- locale is the application's request, and the record's intent paragraph is rewrapped.
…y their standard names
…regorian dates The DateRangeInput record and the changeset say that a provider locale with a -u-ca- keyword keeps Gregorian dates; set calendar to choose one.
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision; no other changes are needed.
DateRangeInput's new calendar prop, its calendar-system trigger text and popover, and the -u-ca- locale keyword now reaching the range text rest on spec:AST-111 FR17 and DEC-2, which are still a draft in #9605, and they depart from current architecture:internationalization INV10, which keeps date grids Gregorian and stops a locale from selecting another calendar for date components; an engineering owner needs to approve or reject:
- the public
calendar?: CalendarSystemprop onDateRangeInput, defaulting to theInternationalizationProvidercalendar, else'gregory'; - the trigger text, its accessible name and the popover Calendar shown in that calendar, with the year when either end leaves the current calendar year;
- a provider locale with an explicit
-u-ca-keyword, such asth-TH-u-ca-buddhist, changing range text that the latest release shows in Gregorian, recorded as nonbreaking; - the proposed record text:
component:DateRangeInputFR6, itscalendarconcept row andspec:AST-013FR12 exception,component:InternationalizationProviderFR10 and its Calendar row, and thecomponent:Calendarrelationship line.
An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Optional: FR6 could name the trigger's accessible name with a test that asserts it, and a DateRangeInput test under a -u-ca- provider locale would pin the one case that changes released text.
[Automated review]
93bca81 to
cdc5efc
Compare
a26bf47 to
37c9150
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision; no other changes are needed.
DateRangeInput's new calendar prop, its trigger text in the application's calendar and its popover grid in that calendar rest only on spec:AST-111 FR17, a draft in #9605, and the grid departs from current architecture:internationalization INV10, which keeps date grids Gregorian; an engineering owner needs to approve or reject:
- the public
calendar?: CalendarSystemprop onDateRangeInput, defaulting to theInternationalizationProvidercalendar, else'gregory'; - the trigger text, its accessible name and the popover Calendar shown in that calendar, with the year when either end leaves the current calendar year;
- the proposed record text:
component:DateRangeInputFR6, itscalendarconcept row, Calendar state row andspec:AST-013FR12 exception,component:InternationalizationProviderFR10 and its Calendar row, and thecomponent:Calendarrelationship line.
An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Optional: FR6 could name the trigger's accessible name, with a test that asserts it.
[Automated review]
This implements
spec:AST-111(draft, #9605) FR17 forDateRangeInput, with FR3 and FR5. The native-fallback (FR15) and bottom-sheet (FR16) parts that this PR used to carry are now the two PRs below it.User need
Under a non-Gregorian provider,
DateRangeInputis the last date component that still shows Gregorian text: a Persian booking form shows "Oct 30 – Nov 2" in its trigger while every other date reads in Persian. People can't read or pick a range in the application's calendar.Problem and solution fit
DateRangeInputtakescalendar, defaulting to the provider's calendar, elsegregory. A-u-ca-keyword in the provider locale does not select the calendar. Its trigger text and popover Calendar use it; the trigger shows the year when either end falls outside the current calendar year. It has no typed entry.min,max, presets and span bounds stay Gregorian ISO (FR3). WithoutTemporal, calendars that need it fall back to Gregorian and warn once in development (FR5).Current specification
spec:AST-111FR17, FR3 and FR5 (draft, spec(AST-111): date components use the calendar the application chooses #9605).component:DateRangeInputFR6, with thespec:AST-013FR12 exception matching DateInput's.component:Calendarandcomponent:InternationalizationProvidernow list DateRangeInput among the components that pass or follow the calendar.Public delta and caller burden
DateRangeInputcalendar?: CalendarSystem, defaulting to the provider's calendar, else'gregory'(API report row added).gregory/iso8601.Caller decisions: none.
calendaris optional.Design and alternatives
calendarprop; it adds no new model.Evidence
onChange, Buddhist years, Gregorian text by default, and the no-Temporalfallback.Temporal, with the polyfill, and with noTemporal; WebKit (Safari 26.5) with the polyfill and with noTemporal.Screenshots
Chromium 149 with native Temporal.
Scope
Testing
pnpm exec vitest run --maxWorkers=2 packages/core/src/Calendar/Calendar.calendarSystems.test.tsx packages/core/src/DateRangeInput/DateRangeInput.calendar.test.tsx packages/core/src/docPropLiterals.test.ts: 3 files, 49 tests passed.pnpm exec tsc --noEmit -p packages/core: passes.pnpm check:knowledge,pnpm check:changesets: pass.pnpm review:pre --base nynexman4464/feat/calendar-bottom-sheet: 474 authored lines, no findings.core-daterangeinput--calendar-systems.Writing rubric results
component:DateRangeInputcalendarconcept, FR6, the Calendar state row, and the AST-013 FR12 exception are present tense.component:Calendar(#2715)is part of a quoted test title.component:InternationalizationProvider