-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5306
imdreamrunner · OPEN · view on GitHub
Verdict: approve with nits
Head reviewed: 03e802e7157a0f13c0c9ef0b248f351693acac5f
With min/max set to a window that does not contain today, the calendar
opened on today's month: every day greyed out and the prev arrow dead, with no
indication of where the selectable dates were.
getInitialFocusDate clamps the opening month into the window, and the
two-month view shifts so the window's months are the ones on screen. Precedence
is unchanged: an explicit focusDate wins, then value, then the clamped
today.
Everyone on the release that ships it, but only where min/max are set and
today falls outside them. Nothing opts in; existing calendars without bounds
render identically.
No API change.
No new theme targets.
- API — no. No signature moves.
- Visual — yes, deliberately: a bounded calendar with no value now opens on a different month than before. That is the fix.
- Theme — no. No target added, moved or renamed.
-
Render — the clamp runs in a
useStateinitializer, once per mount. No new effects, subscriptions or state. - Listeners — none added.
- Layout — no layout reads.
- Bundle — no new dependency.
Needs judgement — no new API surface and no performance regression, but it changes what is already on screen for min/max consumers without a value.
approve with nits
1. The clamp is a mount-time decision (useState initializer), so bounds that
arrive after mount never reach it
→ picking a check-out date after choosing a check-in six months out opens on
today's month, every day greyed, prev arrow dead — the exact bug this PR
fixes, in an ordinary two-field form. Real Chromium: check-in Feb 15 2027,
check-out opens August 2026, 0 of 42 days enabled · Calendar.tsx:287
2. The docs state the fallback with no "on mount"
→ whoever writes `<DateInput min={checkIn} />` reads a promise the component
does not keep, and gets silence, not an error · Calendar.doc.mjs:87
3. A selected value outside the window still wins over the clamp, by design
→ a form loading a saved date that has fallen out of the window opens on that
month with 0 enabled days · getInitialFocusDate.ts:66
Not a block: findings 1 and 3 are the unchanged status quo — the PR neither creates them nor makes them worse. Weighed finding 1 against R1i and it does not tip: static-vs-late-arriving bounds are not two spellings of one declaration, and no general mechanism exists for re-deriving the month when bounds change.
Thanks, this is good. Clamp and the two-month shift both check out. Nit: bounds that arrive after mount — a check-out
min— still open on today's disabled month.
-
packages/core/src/Calendar/Calendar.tsx:287— Mount-only: a check-outminchosen later still opens on today, all disabled. -
packages/core/src/Calendar/Calendar.doc.mjs:87— Worth saying "on mount" — the clamp doesn't re-run when bounds change.
Posted as drafted.