Repository navigation
[Design Polish] Center the search router on wide viewports - #100831
Conversation
Co-authored-by: Shawn Borton <shawnborton@users.noreply.github.com>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
|
Nothing to fix here — every code check on this PR passes. The one failure, It fails because the PR has zero approvals and needs one from a reviewer who isn't a commit author: Since you're listed as a commit author, your own approval won't count. Someone else needs to review and approve it. The two Full check statusAll 30 other checks pass or skip, including Failing step: |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
🚧 shawnborton has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
@MelvinBot can you make the search bar appear all the way on the right edge? This might mean that you need to add the right padding/margin to a different view and remove it from the view that is wrapping the search bar. 
|
|
⏱️ Ran out of time before I could finish. This usually means the task was too complex or I got stuck exploring. Please try again with a more specific request or post in #melvinbot-ai. |
Co-authored-by: shawnborton <shawnborton@users.noreply.github.com> Co-authored-by: Shawn Borton <shawnborton@users.noreply.github.com>
|
Done — pushed in Geometry inside the 600px card: the bar was 16px in from both edges, and is now 16px in on the left and flush on the right. Two things to flag before you look at the build. I couldn't screenshot it. The web test session went blank part way through the run, so this change is verified by code and by The bar has a visible border, so running it flush means its right border now sits exactly on the card edge. If that reads as clipped rather than intentional, say the word — reverting to a symmetric inset is a one-line change, and so is giving it a smaller right inset (8px) that lines up with the left edge of the rows below. Separately, one thing I found that may actually be what you're seeing. The text input element inside the bar is capped at 375px, while the bar itself is now 568px wide: |
|
Re-checked after
Full check status on 25b5fa4Passing: Failing step: |
|
Findings:
@MelvinBot three changes: (1) move the layered shadow into the theme as a token in light.ts/dark.ts instead of hardcoded rgba in searchRouterPopoverShadow, the floating RHP will reuse it and the current layers disappear on dark; (2) fix the 375px cap in SearchAutocompleteInput you found so the input fills the 600px card; (3) keep mh85vh alongside the 520 cap so short viewports don't clip. Drop the three comment blocks while there. |
|
All four are straightforward, and one of them has a trap in it. Here is the exact shape of each — I can't push them, see the note at the bottom. 1. Shadow as a theme token. Add an For the dark values: the existing 2. The 375px cap. SearchAutocompleteInput.tsx:196 is the only place 3. The 85vh floor — this one needs care. Putting 4. The comments. I count five blocks on this branch rather than three — SearchRouterModal:64 and :72, styles/index.ts:1284-1285, variables.ts:248, and the block I added at SearchRouter.tsx:508-512. I'd drop all five unless you want some kept.
|
|
@grgia I will ask Melvin to make your changes but then I think I need to take this one to Claude to better implement the search bar stuff I was trying to do |
|
@MelvinBot implement the changes here please |
|
All good @shawnborton. I'll wait for your call to run some reviews |
…t, clamp the popover height to the viewport Co-authored-by: shawnborton <shawnborton@users.noreply.github.com> Co-authored-by: Shawn Borton <shawnborton@users.noreply.github.com>
|
All three done in 1. Shadow is a theme token. New 2. 375px cap is gone. 3. Height clamps to the viewport. Comments. I dropped five, not three — the two in Checks. Still no browser screenshots. The web test session went blank again — the page dropped to zero rendered nodes while idle, on a healthy dev server, for the second run in a row. So the dark-theme shadow and the full-width input are verified by code, not by eye. The adhoc build will rebuild on this commit if you want to look. |
|
Still nothing for me to fix. The full suite re-ran on the latest commits and 34 checks now pass, including all 8 The single remaining failure is You count as a commit author, so your approval won't satisfy it. The PR needs one approval from someone who isn't you or me. Failing step: |
…ore the input's right margin
|
Pushed my changes @grgia feel free to give this another review now |
…uter Co-authored-by: Shawn Borton <shawnborton@users.noreply.github.com>
|
Fixed and pushed — the branch was one commit behind Two Nothing in this PR touches middleware. All 8 The one remaining failure, Verification before pushingRan locally on the merged branch:
Merge commit: view run · no recording available |
|
🚧 shawnborton has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
grgia
left a comment
grgia
left a comment
There was a problem hiding this comment.
Nothing blocking code wise, only comment is we could add an automated test:
Add a small unit test for
getSearchRouterPopoverLayoutat e.g. 1000, 700 and 500 px heights, asserting topOffset and maxHeight (100/520, 90/520, 16/468).
@MelvinBot wdyt
Co-authored-by: Shawn Borton <shawnborton@users.noreply.github.com>
|
Good call — added in Your three expected pairs are exactly right — I checked each against the formula before writing them down:
I added two things on top of your three cases, both cheap:
To check the test isn't vacuous I mutated the helper (dropped the AI tests run locally on 48dc142
One thing I deliberately left untested: below a 32px viewport height view run · no recording available |
| */ | ||
| function getSearchRouterPopoverLayout(windowHeight: number): SearchRouterPopoverLayout { | ||
| const evenGap = (windowHeight - variables.searchRouterPopoverMaxHeight) / 2; | ||
| const topOffset = Math.min(variables.searchRouterPopoverTopOffset, Math.max(variables.searchRouterPopoverMinTopOffset, evenGap)); |
There was a problem hiding this comment.
@MelvinBot Can we use clamp from NumberUtils for this logic?
There was a problem hiding this comment.
Yes — good catch, applied in ad0b58f.
clamp(value, min, max) is literally Math.min(Math.max(value, min), max) (NumberUtils.ts:68-70), so this is an exact behavioural equivalent — just easier to read:
const topOffset = clamp(evenGap, variables.searchRouterPopoverMinTopOffset, variables.searchRouterPopoverTopOffset);getSearchRouterPopoverLayout.ts:21. NumberUtils has no imports of its own, so there's no cycle risk pulling it into this helper.
I left maxHeight on line 25 as a plain Math.min — it only has an upper bound, so clamp would need a fake lower bound to fit.
AI tests run locally
| Check | Result |
|---|---|
npm run typecheck |
✅ passed |
eslint on the changed file |
✅ passed |
npm run react-compiler-compliance-check check <file> |
✅ passed |
npm test — getSearchRouterPopoverLayoutTest |
✅ 10 tests passed |
npm test — BaseModalTest |
✅ 7 tests passed |
The existing 10 unit tests for this helper pass unchanged, which is the equivalence check.
Couldn't run: npm run spell-changed (its git merge-base against origin/main doesn't resolve in this environment) and npm run prettier (no such script in this repo, and the only resolvable prettier config here isn't App's). The change adds no new words beyond clamp and NumberUtils, both already in the codebase.
view run · no recording available
Co-authored-by: huult <huult@users.noreply.github.com>
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
@shawnborton Could you complete the author checklist? |
|
Done! |
|
All you @carlosmiceli |
|
Not sure why Melv assigned a second engineer review on these tbh |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 carlosmiceli has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/carlosmiceli in version: 9.4.78-0 🚀
|
|
🤖 No help site changes are required. I did not create a docs PR. This PR only changes where the search popover sits and how big it is on wide viewports — centered instead of anchored top-right, a fixed max height, a layered shadow, and a dimmed backdrop. It adds no feature, renames nothing, and changes no user-facing copy, tab name, setting label, or button. The help site never documents the popover's position or size, so nothing in What I checkedDiff surface. All 12 changed files are layout, styles, variables, or tests ( Positional references in the docs. Two New Expensify articles describe where the Search icon lives, not where the popover opens:
The icon stays in the top-right — this PR touches no top-bar file — so both lines remain correct. Line 24 of the operators article says operators go in "the search bar that opens from the Search icon," which is position-neutral and still accurate. Other search articles. Use-Search-Shortcuts.md, Navigating-New-Expensify.md, and Search-and-Download-Expenses.md describe behavior and syntax only. None mentions the popover's placement, width, height, or shadow. Screenshots. No article embeds a screenshot of the search popover, so none went stale. The two search-named images in @shawnborton — since no docs update is needed, there is no help site PR to review. If you'd rather the articles call out the new centered command-palette treatment anyway, reply with view run · no recording available |
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.4.78-3 🚀
Bundle Size Analysis (Sentry): |
| useEffect(() => { | ||
| if (!isSearchRouterDisplayed || shouldUseNarrowLayout) { | ||
| return; | ||
| } | ||
|
|
||
| const subscription = Dimensions.addEventListener('change', () => closeSearchRouter()); | ||
|
|
||
| return () => { | ||
| subscription.remove(); | ||
| }; | ||
| }, [isSearchRouterDisplayed, closeSearchRouter, shouldUseNarrowLayout]); |
There was a problem hiding this comment.
this PR should clean up this useEffect or it caused
Explanation of Change
Centers the search router popover on wide (web/desktop) viewports instead of anchoring it to the top-right corner, so it reads as a modern command palette rather than a dropdown hanging off the search icon.
The prototype in #100187 established this treatment; this PR ports only the search-router slice of it, with no other changes from that PR.
Three deltas, all scoped to
!shouldUseNarrowLayoutso narrow/mobile layout is untouched:popoverAnchorPositionbecomes{left: 0, right: 0, top: 40}on wide layout. ThePOPOVERmodal style already appliesposition: absoluteplusalignItems: 'center', so spanning the anchor box full width is what centers the card; the 40px top offset lifts it off the window edge. Narrow layout keeps{right: 6, top: 6}.mh85vh(85% of viewport height, so the popover grew with the window) with a fixedmaxHeight: 520plusoverflowHidden, so the results list scrolls inside a stable card instead of the card stretching.mh85vhwas only used here.searchRouterPopoverShadowstyle layers three wider, lighter brand-tinted shadows behind the existingtheme.shadow, passed asinnerContainerStyle, for stronger separation from the page now that the card floats over content rather than tucking into a corner.The popover also widens from 512px to 600px, matching the mockup in the issue and the prototype — the measured card in the design is 600px. Say the word if you'd rather keep 512px; it's a one-line revert of
searchRouterPopoverWidth.The input's wide-layout margins go from
mv2/mh2(8px) tomv4/mh4(16px) so the inset stays proportional at the wider size, as in the prototype.Fixed Issues
$ #100823
PROPOSAL: #100823 (comment)
Tests
Offline tests
QA Steps
Same as Tests
AI Tests
Run locally on this branch:
npm run typechecknpm run lint-changedno-unsafe-type-assertionatSearchRouter.tsx:246, untouched by this PR)npm run react-compiler-compliance-check check <changed files>npm run gh-actions-unused-stylesnpm run spell-changednpm test—SearchRouterPageTest,BaseModalTest,SearchAutocompleteListTestCould not run:
npm run react-compiler-compliance-check check-changed(itsorigin/<base>resolution fails in this environment — ran the equivalentcheckagainst the changed files instead), andnpm run storybook -- --smoke-test --ci. The fullnpm testsuite was not run; CI covers it.Browser verification of the rendered result is in the MacOS: Chrome / Safari section below.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Verified on dev NewDot web in a 1600x1000 viewport. Measured from the screenshots: the card spans x=500–1100, so it is exactly 600px wide and centered on the viewport midpoint, with its top edge 40px below the window top.
Corners at 4x — rounded and unclipped, with the layered shadow visible past the card border:
Typing
type:expense— suggestions render inside the card, which stays under the 520px cap, and the focused input sits ~16px in from each card edge (input border at x=516/1083 vs card at x=500/1100):Dismisses cleanly, no leftover overlay:
The test account was freshly created, so the popover has little recent-search content and renders short; the geometry above is what this PR changes.