-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5314
#5314 · imdreamrunner · internal · draft
Head reviewed bdbb6d0f9155 (+2555/−0, 9 files)
Verdict comment — posted
2026-08-23, plus a follow-up comment
on the barrel export.
Comment rather than approve/request-changes because it is a draft
(R1c) — and the review says which verdict it is: undrafting wants
the switch settled.
PowerSearch's desktop shape does not survive a phone: the typeahead drops a popover that fights the on-screen keyboard, and the edit popover lays field / operator / value in a row with nowhere to go at 390px.
A new PowerSearchMobile — same props, same PowerSearchFilter model, same
tokens — replacing the dropdown and row popover with a pinned-tall bottom sheet
that drills down field → operator → value. Two taps on the happy path.
Nobody until a call site opts in — but see the API slot: it lands in the published core barrel, so the shape is public the day it merges.
+ PowerSearchMobile (public — PowerSearch/index.ts is re-exported by
+ PowerSearchMobileProps `export * from './PowerSearch'`, so both reach the
published core barrel)
+ astryx-power-search-mobile → the mobile surface
Frozen as soon as a theme selects on it.
The class question is not the sheet, it is who owns the touch switch. DateInput
settles it inside the component on (pointer: coarse), with no width bound;
#5315 does the same for Tokenizer.
This PR puts it at the call site and teaches a width in the docs. Whichever way it
goes, it should go the same way for all three — and if the answer is "one
PowerSearch picks its own surface", the export shipped here is one we deprecate
immediately. DateInput keeps TouchDateField internal for that reason.
comment — one open question, no blocking defect found in the sheet itself.
1. The call site owns the breakpoint, and the docs teach 768px
→ an iPhone in landscape crosses it, swapping component identity and
unmounting a half-built filter · PowerSearchMobile.doc.mjs:197
2. The component and its props are barrel-exported
→ if the switch moves inside PowerSearch, we deprecate an export we just
shipped, and the theme target is already frozen · PowerSearch/index.ts
Thanks — the sheet is lovely and two taps is the right target.
The hard part is who owns the switch. DateInput settles it inside the component on
(pointer: coarse), no width bound; your #5315 does the same for Tokenizer. Here the call site owns it, and the docs teach a width:const isTouch = useMediaQuery('(max-width: 768px)'); const Search = isTouch ? PowerSearchMobile : PowerSearch;That swaps component identity, so a rotation mid-filter unmounts the half-built filter.
Ideally one
PowerSearchpicks its own surface. The argument the other way is bundle — PowerSearch doesn't pull BottomSheet today. Comment only because it's a draft; undrafting wants this settled.Is the tree-shaking worth the call site owning the breakpoint?
[Reviewed by Robohands]
Inlines (all under 20 words):
PowerSearchMobile.doc.mjs:197— Hmm, 768 is an iPhone in landscape. Swaps the component mid-filter
PowerSearchMobile.tsx:617— Some of this seems copied from PowerSearch. Probably can share it rather than keep two
PowerSearchMobile.tsx:322— These four typecheck but never arrive. Might need a dev warning at least
Follow-up comment, posted five minutes later, once the barrel export was
traced: PowerSearchMobile and PowerSearchMobileProps reach the published core
barrel through export * from './PowerSearch', and landing the sheet unexported
would let the work in without committing the public shape.
Cindy on the first draft: "The inline comments need better voice", then "Basically tell them the problem concisely. If there's an existing pattern, ask them to use it." → R2g.
| drafted | posted |
|---|---|
| "an iPhone in landscape is 852px, so this bound swaps the component under someone mid-filter" | "Hmm, 768 is an iPhone in landscape. Swaps the component mid-filter" |
| "these four never arrive; nothing warns the builder who passes them" | "These four typecheck but never arrive. Might need a dev warning at least" |
The drafted column is the JUDGEMENT → line copied down to the line number. The
consequence belongs in the summary; the inline carries location and the pattern
to use, and hedges — "Hmm", "Might need", "Probably can" — because it was written
by someone who has read one part of the file.