Repository navigation
feat(lab): MobilePowerSearch gains the Core PowerSearch sheet's behavior - #8740
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
PR Analysis Report
|
There was a problem hiding this comment.
Approval is on hold until an engineering owner identifies the spec that governs Lab MobilePowerSearch. No current record covers it, so the Core-parity behavior and semantic alignments 1–5 need that decision: for example, confirming component:PowerSearch DEC-3, FR10–FR21 and AR4–AR6 as its contract and ruling on alignment 4, whose UTC-day tokens and rows differ from FR11/FR12's popover-token zone handling. 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. Two fixes are needed whatever is decided:
- With no
disabledMessage, disabling the field while a sheet is open drops focus to the page body because the trigger keeps the nativedisabledattribute (MobilePowerSearch.tsxline 1724), which keeping onlyaria-disabledwould fix. - The
timezoneIDprop JSDoc (line 209) and file header (lines 42–45) still say it drives date formatting, contradictingMobilePowerSearch.doc.mjs.
[Automated review]
| const [wasDisabled, setWasDisabled] = useState(isDisabled); | ||
| if (isDisabled !== wasDisabled) { | ||
| setWasDisabled(isDisabled); | ||
| if (isDisabled && step != null) { | ||
| setStep(null); | ||
| } | ||
| } |
There was a problem hiding this comment.
When this closes the flow while isDisabled is set and no disabledMessage is given, each sheet returns focus to triggerRef (finalFocusRef), but the trigger then carries the native disabled attribute (disabled={isDisabled && !showsDisabledMessage}, line 1724), so focus falls to <body>. Dropping native disabled from the trigger and keeping aria-disabled would fix it; openManager already returns early while disabled. The disabled-close test could also assert expect(trigger()).toHaveFocus() after settleSheets().
| * - `components[type].Token` overrides receive Core's PowerSearchTokenProps, | ||
| * which carry no `timezoneID`; the built-in token renders Core `Token` | ||
| * directly so the configured timezone reaches tokens and row summaries | ||
| * through the same formatter; |
There was a problem hiding this comment.
Since this change, date tokens and rows read the UTC day and timezoneID no longer reaches them. This header line and the prop's JSDoc at line 209 ("Timezone ID for date formatting in tokens, row summaries, and date editors") now contradict MobilePowerSearch.doc.mjs.
f9c3b11 to
d3b66c9
Compare
There was a problem hiding this comment.
Approval is on hold until an engineering owner identifies the spec that governs Lab MobilePowerSearch: no current record covers it, so the Core-parity behavior and semantic alignments 1–5 need that decision, for example confirming component:PowerSearch DEC-3, FR10–FR21 and AR4–AR6 as its contract, including the date gap that record lists as known and alignments 2 and 4 now bring into Lab (in a zone west of UTC, a picked March 10 shows as "Mar 9, 2024" on the token and row). 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. The two fixes from the previous round are in and nothing else blocks; two optional cleanups:
- The comment above the result-count Effect (
MobilePowerSearch.tsxlines 1497–1500) still describes the frame, clear timer and teardown Effect this change removed. - The timezone test builds its expected text in the runner's zone, so on a UTC runner it would not catch tokens going back to the UTC day; pinning a non-UTC zone and asserting literal text would.
[Automated review]
| @@ -1194,9 +1504,17 @@ export function MobilePowerSearch({ | |||
| return; | |||
There was a problem hiding this comment.
The comment above this Effect (lines 1497–1500) still says announce "starts a frame and a clear timer, which the sheet announcer's teardown Effect and the next write release". This change removed that frame, timer and teardown, so the sentence now misstates the Effect's resources; pointing it at the useAnnounce route is enough.
| (timezoneID, compact, long) => { | ||
| // The text the Core PowerSearch token shows for the instant: the day in | ||
| // the browser's zone, no time of day (`formatDateAbsoluteCompact`). | ||
| const coreTokenText = new Intl.DateTimeFormat('en-US', { |
There was a problem hiding this comment.
This builds the expected text with formatDateDay's own Intl.DateTimeFormat options in the runner's zone, and it.each varies only the timezoneID prop, so on a UTC runner the test also passes if formatDateDay formats in UTC. Pinning a non-UTC zone for this block (at America/Los_Angeles this instant is Mar 9) and asserting the literal text would make it fail on that regression.
Ports what the Core PowerSearch sheet presentation gained into Lab's MobilePowerSearch so the touch flow's UX pass starts from the best version: announcements delivered inside the open sheet by useAnnounce, empty-result and unavailable messages spoken from their rendered element, one focus re-home rule over the button rows, rows and tokens keyed by filter identity, Apply and Cancel after a controlled update removed the edited filter or the opening field, the unavailable state with focus on Remove, stale filters listed by their raw keys with the step titled the same way, Removed and Clear all announcements, the field becoming disabled closing the flow, and endContent keeping its own taps. Semantic alignments with Core (DEC-3): Clear all removes every filter, read-only ones included, as one remove at the last index; date editors read and write the UTC day; isDateOnly: false gets the date-only editor; dates read as the UTC day on tokens and rows.
d3b66c9 to
3259f54
Compare
|
/approve-spec 3259f54 |
There was a problem hiding this comment.
Changes requested: the Core-parity direction is settled by the engineering owner's /approve-spec on this head and the ported behavior checks out, but two of the new in-sheet announcements are spoken from sheets that are no longer open. Gate SheetEmptyMessage on its step being the open one, reset its spoken occasion when hidden, and add a regression test for each path:
- Leaving Add filter (Back, Escape, the scrim) after typing a query that has suggestions announces "No results found", because
contentResultsis emptied wheneverstep !== 'fields'while the closed sheet's body stays mounted (MobilePowerSearch.tsxlines 817 and 1993–1997). - Once the value step has closed, a config update that drops the last edited field or operator announces "This filter is no longer available" with no filter open, and reopening the same stale filter does not announce it again (lines 2144–2147).
[Automated review]
| // the empty-result message carries the results id. | ||
| <SheetEmptyMessage | ||
| id={contentResultsId} | ||
| message="No results found" |
There was a problem hiding this comment.
This message also mounts in the closed Add filter sheet: contentResults is [] whenever step !== 'fields' (line 817) and BottomSheet keeps a closed sheet's body mounted, so leaving with Back or Escape after a query that listed suggestions speaks "No results found" — on the page, or in the Filters dialog after Back. FR18 limits it to a query whose result set is empty. Rendering it only while step === 'fields' (the empty results div keeps aria-controls valid), or gating SheetEmptyMessage with an isShown input, would fix it.
| </Text> | ||
| </div> | ||
| <SheetEmptyMessage | ||
| message="This filter is no longer available" |
There was a problem hiding this comment.
The draft outlives its step and this message has no step guard: after the flow closes, a config update that drops the last edited field or operator speaks "This filter is no longer available" on the page, and reopening the same stale filter speaks nothing because the occasion is unchanged. FR14 ties the message to the filter being edited, inside the sheet; an isShown={step === 'value'} gate that resets the spoken occasion when hidden covers both cases.
feat(lab): MobilePowerSearch takes the Core sheet flow's behaviors
Governing record
component:PowerSearch(packages/core/src/PowerSearch/PowerSearch.spec.md, current, merged in #8000) — DEC-3 and its FR10–FR21, AR4–AR6 — specifies the touch sheet flow: the trigger, the Filters, Add filter and value steps, focus after each step and after a controlled update, the in-sheet announcement route, Clear all's and the date editors' semantics. Lab'sMobilePowerSearchis the prototype that implements that flow outside the stable component so its UX can iterate; this PR brings the prototype up to the record's current behavior where Lab can reach it withmain's public API, and lists the remaining gaps below. Lab has no record of its own; its.doc.mjsis the consumer surface.User need
The touch PowerSearch flow is about to get a UX pass, and that pass should start from the best version of the flow. Two versions exist: Lab's
MobilePowerSearch, the prototype the flow was designed in, and the Core PowerSearch sheet presentation (itsexperimental.presentationoption), which went through many review rounds and gained behaviors the prototype never had — announcements delivered where assistive technology can hear them, one rule for where focus goes when a controlled update removes the control it was on, rows that keep their element across a parent's reorder, stale filters that can still be removed, a field that cannot keep a modal flow open once it is disabled. Why does the gap matter? The owner's pass will be made against Lab, so every behavior Lab lacks is one the pass either re-derives or loses. Why not point the pass at Core instead? Core's sheet is behind an experimental option on the stable component and carries its own review constraints; Lab is where the design iterates. Why not wait for graduation? The pass is now.This PR ports what Lab lacked, aligns the semantics the two components deliberately decided differently (listed below for veto), and names what cannot be ported without a Core change.
Problem and solution fit
Each delta ports one behavior of the Core sheet into Lab, with a test that fails on
mainand passes here:useAnnounce. Lab kept a hand-rolled polite region in each sheet (arequestAnimationFrameclear-then-write with a two-second timer).mainhas the modal-awareuseAnnounce(fix(core): useAnnounce delivers inside the open Astryx modal that encloses its caller #8009): called from inside a sheet it delivers into that sheet's dialog. ASheetAnnouncerrendered in each step publishes the dialog'sannounce; messages queued by a step change (an added suggestion, a removal) are spoken in the commit that shows the next step, so they land in that dialog rather than the one being replaced. The hand-rolled region, its frame and timer are gone. Tests: result count change and "Added …" land in the dialog's own live region, not the page body; two messages in one commit are joined.SheetEmptyMessagethat reads its own text and announces it viauseAnnouncefrom inside the sheet. "No filters match your search" shows only for a non-empty query — a config with no field lists nothing — and the content searchbox'saria-controlsalways names an element (the list, the message, or an empty results region). Tests for each.createFilterIdentitiesinmobilePowerSearchUtils.ts:WeakMapid per object, a clone takes the id of the previous update's filter with the same signature, equal filters told apart by order). A parent's reorder or a removal above the focused row moves elements instead of remounting them. Tests: reorder with the same objects and with clones keeps focus on the same element; removal above keeps focus, not the old index.legacy: was", lists as a row "legacy was" (the field's label when the field remains: "Author was"), opens the unavailable step titled the same way (the dialog's name), and Remove takes it out. Tests.formatDateDay: year, abbreviated month, day, Gregorian calendar, the browser's zone, no time of day —formatDateAbsoluteCompact's text), so a Lab token or row and a Core token read byte for byte the same — see the semantic alignment on dates.Removed {label}after Remove, through the catalog's@astryx.tokenizer.tokenRemovedwith the row's text, spoken in the Filters dialog; Clear all's after-close announcement ("All filters removed", or "All filters removed. {count}" when the count changed in the same commit) spoken on the page once focus is back on the trigger, since the closing dialog is modal through its exit. Tests.disabled—aria-disabledonly, as Core's trigger (spec:AST-085FR1/FR2);openManagerrefuses while disabled — so the closing flow hands focus back to it instead of dropping it on the page body, and a disabled reason can be read from it. Tests: the flow closes and the trigger holds focus (fails without the change); a disabled trigger is focusable and does not open.endContentoutside the trigger: the end lane (result count andendContent) was already outside the trigger button, but a tap on it opened the flow through the wrapper; it now keeps its own taps. Test: a button inendContentfires and the flow stays closed; a tap on a token still opens it.onFocus/onBlurcounts: oneonFocuson the trigger, oneonBlurwhen the flow opens, oneonFocuswhen it closes — Lab already behaved; guarded by a test.@astryx.powersearch.sheet.*keys are not onmain, so Lab keeps its literals (a Lab delta); it uses the existing@astryx.tokenizer.tokenRemovedandtokenAddedand@astryx.powersearch.resultCount.Semantic alignments —
component:PowerSearchDEC-3's deliberate differences, ported so Lab becomes the shipped behavior. Each is one line to veto:onChange([], 'remove', lastIndex), and shows whenever a filter exists (Lab kept read-only filters and reported the first removed index). Test.date_absoluteis the UTC midnight of the chosen day, adate_rangelikewise (Lab converted throughtimezoneID). Test: picking March 9 storesDate.UTC(2024, 2, 9) / 1000.isDateOnly: falsegets the date-only editor (Lab rendered aDateTimeInput). Test.date_absoluteexactly as the Core token does (component:PowerSearchFR11, FR12): the day in the browser's zone, no time of day — the same text Core'sformatTokenValueTextproduces — while the editors keep the UTC day (alignment 2), as Core's do. Core's record carries the zone difference between its token and its editor as a known gap; Lab mirrors Core rather than fixing it on its own. Test (it.eachover Tokyo, Honolulu, unset): the token's and the row's text equal the Core token formatter's output for the instant, the editor reads March 10.timezoneIDno longer changes a date value; the prop stays for API parity with Core, and its JSDoc, the file header and the doc say so.Remaining gaps — cannot be ported without a Core change
nestedvalues: Core's sheet edits them through the popover's built-inNestedEditor, exported only on the Core branch; Lab still leaves nested-only fields out with its development warning.PowerSearchValueEditor's presentation branches (choice rows as radios/checkboxes, the list-value Tokenizer's sheet presentation wired through Core's editor): Lab keeps its ownMobilePowerSearchValueEditor.useAnnounceRenderedTextis not exported from@astryxdesign/core/hooks; Lab'sSheetEmptyMessageis a local equivalent (reads its rendered text, announces once per occasion throughuseAnnounce).@astryx.powersearch.sheet.*) are Core-branch only; Lab keeps literals.formatTokenValueText, locale digits without grouping,Intl.ListFormatconjunction/narrow) lives in Core'sformatTokenValue.ts; Lab's rows and tokens both use Core's publicformatFilterValue, so they agree with each other but not byte-for-byte with Core's token.adaptivepresentation and its latch: Lab has no pointer switching by design, so nothing to port.Current specification
component:PowerSearch(packages/core/src/PowerSearch/PowerSearch.spec.md, current) DEC-3, FR10–FR21, AR4–AR6 describe the sheet flow these behaviors come from; Lab is a prototype with no record of its own and the.doc.mjsis its consumer surface.component:BottomSheetFR5–FR10 own the stacked, replacing step sheets Lab already uses.spec:AST-086FR14 is the announcement routeuseAnnounceimplements.Public delta and caller burden
Props unchanged. Observable: Clear all's result and reporting (above); dates read as the UTC day and the date-only editor for
isDateOnly: false; stale filters now visible and removable; announcements inside the sheets through the shared hook (themobile-power-search-sheet-statustest id is gone with the hand-rolled region); the field becoming disabled closes an open flow; a tap onendContentno longer opens the flow. Doc:hasClear,timezoneIDand the usage paragraph updated.Evidence
MobilePowerSearch114 andMobileTokenizer10 pass. The 27MobilePowerSearchtests that cover ported or aligned behavior fail onmainwith the test file alone (checked by stashing the source files): 13 rewritten for the new semantics (in-sheet regions, Clear all ×3, disabled closes with focus on the trigger, disabled trigger focusable, add announcements ×2, date-only editor, Core-token date text ×3, UTC midnight) and 14 new under "Core parity" (focus re-home ×3, identity keys ×2, stale filters ×2, unavailable state, Apply/Cancel gone ×2, Removed, "No filters match" ×2, end lane). Two parity guards pass onmainas well (aria-controlstarget,onFocus/onBlurcounts).Lab/MobilePowerSearchtouch-flow story): tokens and rows read the date as a day with no time of day (then the UTC day; now the Core token's zone handling — the same text wherever the browser's zone agrees with UTC on that instant); Filters → first row → Remove: "Removed Status is any of Open, In Progress" in the Filters dialog's own live region, focus on "Priority is P1 - High"; Add filter → Back: focus on Add filter; Clear all: flow closed, tokens gone, "All filters removed" in the page region with focus on the trigger; one scrim in every state; no page errors.Scope
MobilePowerSearch.doc.mjs), focused tests (26 before/after) ship with the feature; no migration — props unchanged.Testing
pnpm exec vitest run packages/lab/src/MobilePowerSearch packages/lab/src/MobileTokenizer— 2 files, 124 tests pass; onmain's sources with theMobilePowerSearchtest file, 27 fail.pnpm -F @astryxdesign/lab typecheck:docs— exit 0 (afterpnpm -F @astryxdesign/core buildandpnpm -F @astryxdesign/lab build, which the fresh worktree needed for its type resolution).pnpm -F @astryxdesign/storybook typecheck— exit 0 (afterpnpm build).pnpm check:repo— exit 0; strict lint onpackages/lab/src/MobilePowerSearch/— 0 findings; prettier andgit diff --checkclean.