-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5261
#5261 · imdreamrunner · internal
Head reviewed d4d2cf3553d2
Verdict hold for a ruling — not posted. The PR's stated problem was fixed
two days earlier, by the same author, in merged
#5243.
As the PR states it: on a phone, DateInput's calendar toggle opens a 320px month grid in a popover — a desktop control shrunk onto a screen whose platform already ships a date picker, with system-sized hit areas and the OS locale, calendar and accessibility settings applied for free.
That problem is closed on main. #5243
(merged 22 Aug) split the component: DateInput reads useMediaQuery(TOUCH_POINTER_QUERY)
and renders TouchDateField when the pointer is coarse, PointerDateField when
it is not (DateInput.tsx:888). A phone already gets the OS picker.
This branch predates that split. It adds useNativeDatePicker and a nativePicker
prop ('touch' | 'always' | 'never') to the single pre-split DateInput, with the
format overlay, showPicker() handling and the iOS uncontrolled-input work
described at length in the body.
After a clean merge with main, nativePicker does nothing on touch. The
useNativeDatePicker call sits in the function that main has renamed
PointerDateField (DateInput.tsx:558 on this head; :389 on main), and
PointerDateField renders only when isTouch is false. So on the devices the
prop exists for, the branch that would consult it is never mounted. 'never'
becomes a no-op and 'always' reaches only desktop.
This is a merge-shape finding, not a defect in the branch as written: on its own base the code does what it says.
+ DateInput.nativePicker?: DateInputNativePicker = 'touch' (public, core barrel)
+ type DateInputNativePicker (public, re-exported from
./useNativeDatePicker)
+ useNativeDatePicker() (internal, new module)
None new.
Not carried to a conclusion — the review stopped at the level-1 question (is this the right thing to do at all, now that main does it), which is where R1b says to spend it. No frames captured.
hold for a ruling — nothing to post until the ruling lands. Requesting changes would ask the author to rebase and re-verify ~1,500 lines whose reason for existing is the open question.
1. The stated problem is already fixed on main by the author's own #5243
→ the PR as titled has nothing left to do · DateInput.tsx:888 (main)
2. Merged with main, the prop is unreachable on touch
→ useNativeDatePicker sits inside PointerDateField, which renders only when
isTouch is false · DateInput.tsx:558
Whether an escape hatch survives the split — nativePicker="never" (a field that
looks identical on every device) and nativePicker="always" (native on desktop
too) are the only parts of this PR main does not have, and they are a prop-tier
override of a decision the component now makes from the pointer type. That is a
tier question, so it is Cindy's, not the review's. If the answer is no, this
closes as superseded under R6e — and R6e's test applies to the
leftovers: name who reaches them.
Recorded here only. A superseded close and a request-changes are different comments to write, and which one it is depends on the ruling.