Repository navigation
feat(lab): MobilePowerSearch's steps are standalone sheets that replace one another - #8476
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.
|
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsMobilePowerSearch (@astryxdesign/lab) · View in Storybook
Component Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Accessibility AuditStatus: No accessibility violations detected. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
No blocking issues. Moving MobilePowerSearch's Filters, Add filter and value steps onto standalone BottomSheets follows the documented replacement for the deprecated BottomSheetSwitcher, and the flow renders, dismisses and returns focus the same way as before. Nothing more is needed beyond landing this after #7648, which provides the replace handoff the steps rely on. One optional tidy-up: the test titled "no BottomSheetSwitcher remains" only asserts that Filters opens as one modal dialog, so it also passes with the switcher, and a title saying what it checks would be more accurate.
[Automated review]
| return Array.from(document.querySelectorAll<HTMLElement>('dialog[open]')); | ||
| } | ||
|
|
||
| it('no BottomSheetSwitcher remains: every step is a standalone sheet', () => { |
There was a problem hiding this comment.
This test only asserts that Filters opens as one modal dialog named "Filters", so it also passes against the switcher version. The removed import proves the switcher is gone, not this test. Consider a title that says what it checks, for example "Filters opens as one modal dialog".
| tapRow('Add filter'); | ||
| // The leaving Filters dialog asks to close (as a late Escape would); the | ||
| // flow is on Add filter now, so nothing happens. | ||
| fireEvent(filters, new Event('cancel', {cancelable: true})); |
There was a problem hiding this comment.
BottomSheet already ignores a cancel from a sheet that is no longer open (dismissOnEscape requires isOpen), so this test passes even without the stepRef.current === sheetStep guard in handleStepOpenChange. Sending the stale request as a click on filters (the scrim path, which BottomSheet does not gate on isOpen) would make the test depend on that guard.
b9dbd5f to
9b3436b
Compare
There was a problem hiding this comment.
No blocking issues. Moving MobilePowerSearch's Filters, Add filter and value steps onto standalone BottomSheets follows the documented replacement for the deprecated BottomSheetSwitcher, and the flow's dim, dismissal and focus behave as before on fine and coarse pointers. Please land this only with or after #7648, which provides the replace handoff the steps rely on and can then drop its switcher record's note that MobilePowerSearch still uses the switcher. One optional tidy-up: the "a step on its own has one modal dialog and no stack attribute" test only checks that the stack attributes are absent, so asserting aria-modal and a single open dialog would make it match its title.
[Automated review]
| expect(isSheetOpen()).toBe(true); | ||
| }); | ||
|
|
||
| it('a step on its own has one modal dialog and no stack attribute', () => { |
There was a problem hiding this comment.
Optional: this test's title promises one modal dialog, but it only asserts that data-sheet-stack and data-sheet-stack-depth are absent. No current spec record makes those attributes contractual, and the test passes unchanged against the switcher version. Asserting aria-modal="true" and a single dialog[open] would make it check what its title says.
9b3436b to
9e39033
Compare
eeaa501 to
933e20e
Compare
There was a problem hiding this comment.
No blocking issues. Moving MobilePowerSearch's Filters, Add filter and value steps onto standalone BottomSheets follows the documented replacement for the deprecated BottomSheetSwitcher, and the flow looks, dismisses and returns focus as before, with identical frames on fine and coarse pointers. Please land this only with or after #7648, which provides the replace handoff the steps rely on and can then drop its switcher record's note that MobilePowerSearch still uses the switcher. Optional: the handoff and lone-step tests also assert Core's internal data-sheet-stack marks, which no spec makes contractual, so they would break on a Core rename even while the behaviour holds.
[Automated review]
| // accessibility tree; the entering step is the interactive one. | ||
| expect(filters).toHaveAttribute('inert'); | ||
| expect(filters).toHaveAttribute('aria-hidden', 'true'); | ||
| expect(filters).toHaveAttribute('data-sheet-stack', 'replaced'); |
There was a problem hiding this comment.
Optional: data-sheet-stack is Core's private mark, and no current spec record makes it contractual, so this assertion ties the Lab test to Core's DOM. It is also the only line that makes this test fail against the switcher version; the inert, aria-hidden and presented-dialog assertions around it already prove the handoff.
9e39033 to
c91a138
Compare
933e20e to
c2b66f5
Compare
c91a138 to
4f4311e
Compare
c2b66f5 to
b8eef54
Compare
There was a problem hiding this comment.
No blocking issues. Moving MobilePowerSearch's Filters, Add filter and value steps onto standalone BottomSheets follows the documented replacement for the deprecated BottomSheetSwitcher, and the flow looks and hands off identically on fine and coarse pointers, and dismisses and returns focus as before. Please land this only with or after #7648, which provides the replace handoff the steps rely on and can then drop its switcher record's note that MobilePowerSearch still uses the switcher. One correction for the description: the new handoff test also passes against the switcher version, so it pins shared behaviour rather than the migration; only the stale-dismissal test is sensitive to the new guard.
[Automated review]
| expect(sheet()).toHaveAttribute('aria-modal', 'true'); | ||
| }); | ||
|
|
||
| it('Filters → Add filter is a replace handoff: the entering step is on top, the leaving step stays presented, inert and hidden, then leaves', () => { |
There was a problem hiding this comment.
This test also passes against the switcher version of the component (the base commit's MobilePowerSearch.tsx under this file's tests): both versions keep the two dialogs open with Filters inert and aria-hidden, then leave only Add filter. That makes it a good parity pin, but the description's "fails on the switcher version" no longer holds now that the data-sheet-stack assertion is gone; of the eight new tests, only the stale-dismissal one turns red, and only when the per-step guard is removed.
b8eef54 to
dd9109f
Compare
4f4311e to
0a0ac2a
Compare
dd9109f to
3e00dab
Compare
0a0ac2a to
fd9216f
Compare
There was a problem hiding this comment.
No blocking issues: moving MobilePowerSearch's Filters, Add filter and value steps onto standalone BottomSheets follows the documented replacement for the deprecated BottomSheetSwitcher, and the flow looks, hands off, dismisses and returns focus as before on fine and coarse pointers. Please land it only on top of #7648, which provides the replace handoff the steps rely on and can then drop its switcher record's note that MobilePowerSearch still uses the switcher. One small description fix: grep -rn BottomSheetSwitcher packages/lab/src now matches only the new sentence in MobilePowerSearch.doc.mjs, not a test.
[Automated review]
fd9216f to
525e267
Compare
3e00dab to
1797865
Compare
There was a problem hiding this comment.
No blocking issues: moving MobilePowerSearch's Filters, Add filter and value steps onto standalone BottomSheets follows the documented replacement for the deprecated BottomSheetSwitcher, and the flow looks, hands off, dismisses and returns focus exactly as before on fine and coarse pointers. Please land it only on top of #7648, which provides the replace handoff the steps rely on and can then drop its switcher record's note that MobilePowerSearch still uses the switcher. One small description fix: grep -rn BottomSheetSwitcher packages/lab/src now matches only the new sentence in MobilePowerSearch.doc.mjs, not a test.
[Automated review]
1797865 to
20ed66a
Compare
525e267 to
f95793f
Compare
There was a problem hiding this comment.
Looks good to approve: moving MobilePowerSearch's Filters, Add filter and value steps onto standalone BottomSheets follows the documented replacement for the deprecated BottomSheetSwitcher, and the flow looks, hands off, dismisses and returns focus exactly as before by touch, mouse and keyboard. Please land it only on top of #7648, which provides the replace handoff the steps rely on. Two small description fixes: grep -rn BottomSheetSwitcher packages/lab/src now matches only the new sentence in MobilePowerSearch.doc.mjs (no test names the switcher), and Filters and Add filter each render one element fewer than the switcher version, so the element counts are not identical.
[Automated review]
…ce one another The Filters, Add filter and value steps were children of the deprecated BottomSheetSwitcher. Each is now its own BottomSheet controlled by the one `step` value: a step change closes one sheet and opens the next in the same update, which BottomSheet treats as a replace handoff — the entering step rises above the leaving one under the one scrim, the leaving one fades out inert and hidden, and the opener recorded for the first step carries to each next step so a Done, Back, Escape, scrim press or swipe at the end returns focus to the trigger. A sheet only ever asks to close; only the current step's request ends the flow, so a stale request from a leaving sheet does nothing. Nothing else changes: the sticky header and footer, the trigger's aria-labelledby, the in-sheet role=status regions (still needed while useAnnounce's in-modal route is unimplemented), the StrictMode count guard, the joined pending messages and the ref resets stay as they were. No BottomSheetSwitcher import remains in Lab. Completes the Lab migration DEP-0015 leaves optional (spec:AST-017 FR29c).
f95793f to
42eccc0
Compare
There was a problem hiding this comment.
Looks good to approve: moving MobilePowerSearch's Filters, Add filter and value steps onto standalone BottomSheets follows the documented replacement for the deprecated BottomSheetSwitcher, and the flow looks, hands off, dismisses and returns focus exactly as before by touch and with a mouse. Two small description fixes: grep -rn BottomSheetSwitcher packages/lab/src matches only the new sentence in MobilePowerSearch.doc.mjs (no test asserts the switcher's absence), and each open step renders one element fewer than the switcher version, so the element counts are not identical. Once this lands, the BottomSheetSwitcher record's sentences saying MobilePowerSearch still uses the switcher need their own edit, since #7648, where the description defers it, has already merged.
[Automated review]
Maintainer impact
Lab's
MobilePowerSearchwas the last surface in the repository composingBottomSheetSwitcher, whichcomponent:BottomSheetSwitcherDEP-0015 deprecates in favour of standalone step sheets. The deprecation patch leaves Lab's migration optional (@astryxdesign/labisprivateand published only under@canary, sospec:AST-017FR29c's "maintained Astryx packages" does not reach it); this change completes it anyway, so that nothing in the repository still teaches or depends on the switcher and the Lab prototype exercises the same stack and replace machinery Core ships.Targets
main. It needs BottomSheet's open-order stack and same-update replace handoff, which landed with #7648; this PR is the one Lab commit on top of that merge.Intended invariant
component:BottomSheetFR10–FR11 (a same-update close and open is a replace handoff: the entering sheet rises above the retained one under the one scrim, the retained one fades inert and hidden, focus follows the person to the entering step and returns to the flow's opener when the last step closes) and FR5–FR9 (stacking) — the behaviour the steps now rely on.component:BottomSheetSwitcherDEP-0015 (Lab'sMobilePowerSearchmigrates "by its own change": this one).spec:AST-017FR59 (touch work changes fine-pointer behaviour only for a defect or deliberate consistency): the component is the same on every pointer, and the evidence below shows identical behaviour and element counts under a coarse and a fine pointer.MobilePowerSearchalready promised stays as it was: focus moves and returns (feat(lab): MobilePowerSearch edits list values through MobileTokenizer #7979), Back, swipe and scrim dismissal bypurpose, the sticky header and footer above the rows scrolling under them (fix(lab): MobilePowerSearch's sticky sheet header stays above the rows scrolling under it #8005), the trigger'saria-labelledby, the in-sheetrole=statusregions (data-testid="mobile-power-search-sheet-status"), the StrictMode count guard, joined pending messages and the ref resets on teardown (fix(lab): MobilePowerSearch names its trigger by what it shows and announces inside its sheets #8007).Change
The three steps — Filters, Add filter and the value step — were children of one
BottomSheetSwitcherdriven bystep. Each is now its ownBottomSheet,isOpen={step === '<step>'}, controlled by the samestepvalue, so every step change the handlers already made (setStep('fields'),setStep('manage'), …) is now a same-update close and open that BottomSheet treats as a replace handoff. The switcher'sonActiveSheetChange(which only ever reportednull, for an allowed dismissal) becomes oneonOpenChangeper step that ends the flow only when the request comes from the current step, so a stale request from a sheet that is leaving does nothing.openManagerstill focuses the trigger before the first step opens, and the stack carries that recorded opener from step to step, so focus returns to the trigger however the flow ends. The pending-focus effects (focusPendingSheetTarget) are untouched: they run after the entering sheet's own focus, as before.Why maintenance: no prop, type, export or behaviour of
MobilePowerSearchchanges for a Lab caller; the component only stops composing a deprecated component. No Core or spec file is touched.Announcements.
spec:AST-086FR14 (owners inside an open modal deliver inside it) is onmain, butuseAnnouncestill writes only to the page-body channel (the record names that as the gap), so the in-sheetrole=statusregions are still the only route for a message while a step is open and are kept as they were. Nothing double-announces:announce()routes to the page channel only while no step is open. OnceuseAnnounceimplements FR14, these regions and the component's own routing can go in one follow-up.Follow-up record edit (not changed here):
component:BottomSheetSwitcher's Intent gap note and DEP-0015downstreamrow say Lab'sMobilePowerSearchkeeps the switcher until its own change migrates it; once this lands, that sentence and the remainingImplementation gap:line can be removed from the record in #7648 (the record is its PR's to amend).Evidence
grep -rn BottomSheetSwitcher packages/lab/srcfinds only the test that asserts its absence; the flow runs on BottomSheet's stack. Chromium, static Storybook of this head,lab-mobilepowersearch--touch-flow, 390×844: 32 of 32 checks pass under both the iPhone profile (pointer: coarse) and a desktop profile (fine pointer), among them — tap opens Filters as one modal sheet; during Filters → Add filter both steps are presented, Filtersinert+aria-hiddenwithdata-sheet-stack="replaced", Add filter interactive on top; the page dim is constant through the handoff (0.50 min/max over 79 frames: one scrim); the handoff completes with only Add filter open and focus inside it; choosing Status replaces Add filter with the value step; Cancel returns to Add filter with focus on the Status row; Back returns to Filters with focus on Add filter; Done closes and focus is on the trigger (the return target survived three handoffs); Escape closes to the trigger; a scrim press closes Filters (info) and does not close the value step (form); a finger swipe on the handle dismisses the flow (coarse); the sticky header has its ownz-indexinside an isolated layer (fix(lab): MobilePowerSearch's sticky sheet header stays above the rows scrolling under it #8005); the trigger'saria-labelledbyresolves and the open step carries arole=statusregion (fix(lab): MobilePowerSearch names its trigger by what it shows and announces inside its sheets #8007).Scope
Testing
pnpm exec vitest run packages/lab/src/MobilePowerSearch/— 94 passed (the 86 existing tests unchanged apart from two helper comments that described the switcher, plus 8 new: Filters opens as one modal dialog named by the step; Filters → Add filter is a replace handoff (entering on top and exposed, leaving step presented butinertandaria-hidden, then gone after its fade — behavioural assertions only, no Core-internal attribute), a parity pin: the switcher version produces the same observable sequence, so this test guards the migration against regressions rather than distinguishing the two; a lone step is one open modal dialog (aria-modal="true"), interactive and exposed; focus returns to the trigger after a flow that crossed every step; Escape on Filters closes to the trigger; a scrim press closes Filters (info) and not the value step (form); a touch swipe on Filters dismisses; a stale dismissal from the leaving sheet — a scrim press on the replaced Filters dialog, the one dismissal path BottomSheet does not gate on its open state — cannot end the flow — the one test that fails with the per-step guard removed, the only new logic this migration adds).typecheck, coretypecheck:docs, labtypecheck:docs, storybooktypecheck, a11y-spectypecheck, clitypecheck:strict, clitypecheck:template-docs): each exits 0.ASTRYX_STRICT_LINT=1 eslinton the Lab component and its story: 0 problems.pnpm check:repo✓,pnpm check:knowledge✓,pnpm build✓, Storybook build ✓,build-previews784 routes verified,git diff --check✓.@astryxdesign/labis private and canary-only and ships none (as fix(lab): MobilePowerSearch's sticky sheet header stays above the rows scrolling under it #8005 and fix(lab): MobilePowerSearch names its trigger by what it shows and announces inside its sheets #8007 did).