Improve “Also matches in other modes” phone spacing and tap target - #1604
Improve “Also matches in other modes” phone spacing and tap target#1604BigSimmo wants to merge 11 commits into
Conversation
Restyle the collapsed “Also matches in other modes” disclosure as a raised card with a 48px tap target, leading icon, and clearer subtitle so it sits above the home-indicator edge instead of as a flush bottom bar.
📝 WalkthroughWalkthroughThe PR refines the related-matches UI, preserves its mobile card styling, removes its style-contract exemption, and updates UI smoke tests for responsive geometry, navigation, rail behavior, and document search actions. ChangesResponsive related-matches UI
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
This pull request has been ignored for the connected project Preview Branches by Supabase. |
CI triageCI failed on this PR. Automated classification of the 3 failed job(s):
Compared with main CI run #8077 (cancelled). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
Give the disclosure a bit more clearance above the home indicator when scroll-hidden dock reserve collapses to zero.
The icon sits inside a decorative chevron wrapper and needs an explicit aria-hidden on the Lucide component for the local eslint rule.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Restore fade-up entrance motion, keep empty counts as ellipsis, hide the phone subtitle from the disclosure accessible name, and top-align the 48px View-all hit box with the mode label. Preserve raised-card styling in prescribing by insetting instead of flattening. Count phone bottom margin in the answer overflow smoke budget so clearance is not stolen from the rounding slack. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Prescribing also-matches is now margin-inset only (no unlayered visual border/shadow override), so the cascade-layer inventory no longer lists the class and the exemption entry must go. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Point the inventory walk-back note at a current multi-line selector list after medication-also-matches stopped sharing a visual rule block. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Prefer this branch's larger phone chevron hit target and the matching ui-smoke overflow budget over main's pre-redesign chrome.
| await page | ||
| .locator(".fixed.inset-0.z-\\[100\\]") | ||
| .first() | ||
| .click({ position: { x: 8, y: 8 } }); |
There was a problem hiding this comment.
🔴 Recently merged browser-test updates were undone, so the browser test suite now fails
Updates that had already landed on the main line are wiped out again (tests/ui-smoke.spec.ts:1540-1543 restores a stale backdrop selector) when the branch was merged, so the browser test run fails and several checks that were just added disappear.
Impact: The Chromium browser check goes red on this branch and coverage that was shipped days ago silently vanishes.
Merge commit c593e62 took the branch side of tests/ui-smoke.spec.ts, discarding every change from #1616
git diff f4448f8c..08595cc0 -- tests/ui-smoke.spec.ts (PR #1616, the merge base) and git diff 08595cc0..HEAD -- tests/ui-smoke.spec.ts are exact inverses: the merge resolved the file entirely to the pre-#1616 version. Concretely, the restored assertions no longer match the current source:
tests/ui-smoke.spec.ts:1540-1543clicks.fixed.inset-0.z-\[100\], but the sheet backdrop is nowz-[var(--z-modal)](src/components/ui/sheet.tsx:342), so the locator matches nothing and the click times out. ui: complete and activate the v2 design system #1616 replaced it withmodeSheet.locator("..").tests/ui-smoke.spec.ts:2488-2490targetscross-mode-links-railand assertsmd:flex, but at 1280px the answer surface rendersvariant="responsive-compact"(src/components/clinical-dashboard/answer-result-surface.tsx:307), wherecross-mode-links-railis themd:hiddenphone rail and the wide rail iscross-mode-links-card-rail(src/components/clinical-dashboard/cross-mode-links.tsx:230-246).toBeVisible()fails.tests/ui-smoke.spec.ts:3413re-hardcodesactionFontSize === "14px", but the actions use!text-smand v2 sets--text-sm: 0.8125rem(13px) insrc/app/ckb-v2-tokens.css:64.- The
document frame stretches canvas and native owners at phone and desktoptest plusexpectDocumentOwnerFillsFrame, and the 48px tap-target assertions on the cross-mode rail, are deleted outright. - The scroll-budget comments/values (
permittedOverflow,maxOffset < 200,postCollapseMaxOffset <= 72) are reverted to the pre-ui: complete and activate the v2 design system #1616 numbers.
Prompt for agents
The merge commit c593e624 ("merge origin/main: keep also-matches phone tap-target redesign") resolved tests/ui-smoke.spec.ts entirely to the branch side, discarding every change PR #1616 made to that file on main. Verify with: git diff f4448f8c..08595cc0 -- tests/ui-smoke.spec.ts versus git diff 08595cc0..HEAD -- tests/ui-smoke.spec.ts — they are inverses. Redo the merge for this file by taking main's version (08595cc0) as the base and re-applying only this PR's intended edit (including vertical margins in the alsoMatchesHeight measurement in the 'phone short answer stays top-aligned' test, and any budget adjustment justified by the new max-sm:mb-4). Restored stale assertions that no longer match the source: the '.fixed.inset-0.z-[100]' backdrop locator (sheet backdrop is now z-[var(--z-modal)]), 'cross-mode-links-rail'/'md:flex' in the desktop cross-mode-links test (the wide rail is 'cross-mode-links-card-rail' under the responsive-compact variant), the hardcoded '14px' action font size (--text-sm is now 0.8125rem), plus the deleted 'document frame stretches canvas and native owners' test, expectDocumentOwnerFillsFrame helper, and the 48px tap-target assertions.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/clinical-dashboard/universal-search-also-matches.tsx (1)
138-151: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRender static header content on desktop.
When
isWideis true, this remains a native button.tabIndex={-1}removes sequential keyboard focus, andpointer-events-noneblocks pointer input. Screen reader navigation can still expose and activate a button whose handler has no effect.Render a non-interactive header at
smand render the disclosure button only belowsm. This keeps the desktop header inert for all users.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/clinical-dashboard/universal-search-also-matches.tsx` around lines 138 - 151, Update the header rendering around the disclosure button so `isWide` renders static, non-interactive content instead of a native button, while narrower layouts continue rendering the existing disclosure button and toggle behavior. Preserve the current desktop styling and panel relationship, but remove the desktop-only button semantics, `tabIndex`, and inert click handler from the wide variant.
🧹 Nitpick comments (1)
src/app/globals.css (1)
1667-1669: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace literal spacing values with existing theme tokens.
Lines 1668-1669 introduce
1remand0.25remas direct design values. Use the repository’s@themespacing tokens in both the fallback gutter and the safe-area calculation.As per coding guidelines, “Use Tailwind 4
@themetokens in src/app/globals.css … rather than introducing hardcoded design values.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/globals.css` around lines 1667 - 1669, Update the .medication-also-matches margin-left and margin-right declarations to replace both literal 1rem and 0.25rem values with the corresponding existing `@theme` spacing tokens, preserving the current max() and safe-area calculation behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/components/clinical-dashboard/universal-search-also-matches.tsx`:
- Around line 138-151: Update the header rendering around the disclosure button
so `isWide` renders static, non-interactive content instead of a native button,
while narrower layouts continue rendering the existing disclosure button and
toggle behavior. Preserve the current desktop styling and panel relationship,
but remove the desktop-only button semantics, `tabIndex`, and inert click
handler from the wide variant.
---
Nitpick comments:
In `@src/app/globals.css`:
- Around line 1667-1669: Update the .medication-also-matches margin-left and
margin-right declarations to replace both literal 1rem and 0.25rem values with
the corresponding existing `@theme` spacing tokens, preserving the current max()
and safe-area calculation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 14d19d58-fa93-47c1-a34a-47d3afc4c353
📒 Files selected for processing (4)
src/app/globals.csssrc/components/clinical-dashboard/universal-search-also-matches.tsxtests/helpers/style-contracts.tstests/ui-smoke.spec.ts
BigSimmo
commented
Aug 5, 2026
Closing in favour of #1629, which carries this PR's work forward on top of current Why this branch could not simply be fixed forward. Merge commit — the branch had re-hardcoded What was salvaged (verified applying cleanly onto current
One defect fixed rather than inherited. The desktop "Across Clinical KB" label used Verification on #1629: The branch Generated by Claude Code |
… (#1629) Re-applies PR #1604's intended change on top of current main. That PR's merge commit c593e62 resolved tests/ui-smoke.spec.ts entirely to the branch side, discarding every change #1616 had already landed on main -- CI confirmed it, failing on a re-hardcoded 14px action font size against the current --text-sm of 0.8125rem. Only the three files carrying real work are carried over: - universal-search-also-matches.tsx: raised card, min-h-tap toggle with Layers icon, count badge and subtitle, subtitle excluded from the accessible name, phone bottom clearance. - globals.css and tests/helpers/style-contracts.ts: supporting tokens and contract entries. tests/ui-smoke.spec.ts gets only the one edit #1604 actually intended -- including vertical margins in the alsoMatchesHeight measurement, so the phone bottom margin does not eat the phantom-scroll budget. It is applied to main's current version of that test, so the +61/-152 revert is gone and the file stays at +13/-2. Also fixes a defect the original carried: the desktop "Across Clinical KB" label used --text-soft, a deprecated alias of --decoration-soft that the design system pins below 4.5:1 and bans on text nodes. It now uses --text-muted, matching the sibling subtitle. This is what made #1604 red on check:design-system-contract. Co-authored-by: Claude <noreply@anthropic.com>
Summary
min-h-tap(48px) hit target, leading Layers icon, count badge, and short subtitle so it is easier to find and tap.max-sm:mb-4) as content spacing below the last interactive section (not a chrome/dock reserve restore — those stay0remwhen scroll-hidden).motion-safe:animate-fade-upentrance motion.…(no literal0).…badge) from the disclosure accessible name.items-start+pt-0.5).mb-4does not steal the 24px rounding slack.Verification
npm run test:focused -- --files src/components/clinical-dashboard/universal-search-also-matches.tsx— 50 passednpm run test -- --run tests/audit-navigation-auth-regressions.test.ts tests/favourites-auth-gate.test.ts tests/style-contract-registry.test.ts— 27 passed (re-confirmed after main merge: style + audit 26 passed)npx eslint src/components/clinical-dashboard/universal-search-also-matches.tsx --max-warnings 0— cleannpm run test:e2e -- tests/ui-universal-search.spec.ts --project=chromium --grep "loads submitted cross-mode matches on phones only after expansion"— 1 passednpm run test:e2e -- tests/ui-smoke.spec.ts --project=chromium --grep "phone short answer stays top-aligned with no phantom scroll into black"— 1 passed (✓ … (965ms))/forms?q=acamprosate&run=1(390×844): raised card, Layers icon, subtitle, fade-up class,marginBottom: 16px/?mode=prescribing&q=acamprosate&run=1(390×844): raised card retained (border-radius: 16px, inset shadow, horizontal inset)/Also matches in other modes/and does not match/related modes// subtitle; count badge shows2when matches existnpm run verify:pr-local— not stacked after focused + targeted Chromium proof aboveRisk and rollout
Notes
Desktop (
sm+) behaviour is unchanged: the panel stays always-open and the header remains inert. Prescribing phones keep the raised card via horizontal inset rather than the old edge-to-edge flatten.Also matches collapsed on Forms phone
Also matches expanded on Forms phone
Also matches raised card retained on Prescribing phone
To show artifacts inline, enable in settings.
Summary by CodeRabbit
New Features
Bug Fixes