Align tab bar, top padding, and search box location across Home/Inbox/Spend/Workspaces on mobile - #100881
Conversation
shawnborton
changed the title
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.
|
|
@MelvinBot mark the PR ready for review, fill out the author checklist, add QA/Testing/Offline steps as well |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| <View style={[styles.flex1, styles.border, styles.borderRadiusComponentNormal, styles.appBG, styles.p2, styles.justifyContentCenter, styles.h11]}> | ||
| <Text | ||
| style={[{color: theme.textSupporting}]} | ||
| style={[styles.textLabel, {color: theme.textSupporting}]} |
There was a problem hiding this comment.
❌ UI-3 (docs)
This modified line passes an inline style object literal {color: theme.textSupporting} to the style prop. Inline objects create a new object every render (breaking memoization) and bypass the shared style system. A shared equivalent already exists (styles.textSupporting is defined as exactly {color: theme.textSupporting} in src/styles/index.ts).
Replace the inline object with the shared token:
<Text
style={[styles.textLabel, styles.textSupporting]}
numberOfLines={1}
>Reviewed at: 559ed7a | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
Done — this PR is out of draft, the author checklist is fully checked, and the Tests / Offline tests / QA Steps sections are filled in. What changed in the description
The 🤖 Web test steps verification results: I drove the
|
|
|
||
| import {SearchActionsBarSwitch, SearchFiltersBarSwitch, SearchPageInputSwitch, SearchTypeMenuSwitch} from './Switches'; | ||
|
|
||
| const TOO_CLOSE_TO_TOP_DISTANCE = 10; |
There was a problem hiding this comment.
Ummm no idea.... @MelvinBot why would this have been removed?
There was a problem hiding this comment.
Dug into it — dropping it was deliberate, and the description now has a "Scroll behavior on Spend" section covering it.
The constant gated the header on contentOffset.y > 10, so the tab bar and search input held still for the first 10px of every scroll. The lag then stuck around, because the handler subtracts only the current frame's delta from topBarOffset. The 10px skipped while the gate was closed never gets made up, so the header trailed the content by that much for the rest of the collapse. That is the gap you see when you start scrolling.
On the file you linked: usePreventScrollOnKeyboardInteraction is called only from ActionSheetAwareScrollView/index.ios.tsx and index.android.tsx. App.tsx mounts just the provider, not the scroll view, and the narrow Spend list does not render through that component. So the hook is not in this code path. Tell me if you were pointing at something else.
If the worry is the header reacting to keyboard-driven scroll near the top, this screen already covers that another way: the input's onFocus resets topBarOffset to searchHeaderDefaultOffset, so focusing the search box snaps the header back open.
If you still want a guard against small spurious scrolls, I would gate on the size of the delta rather than the absolute offset. That keeps the noise protection without costing the first 10px. Name the case you have in mind and I will switch to it.
There was a problem hiding this comment.
(From my homie Claude)
There was a problem hiding this comment.
It came out in 559ed7a ("Move the narrow Spend header on the first pixel of scroll"), which was mine, not yours. That constant had exactly one use — the scroll gate at index.tsx:133 — and that commit changed contentOffset.y > TOO_CLOSE_TO_TOP_DISTANCE to contentOffset.y > 0, so the constant went unused and got deleted with it.
My recommendation: revert that commit. It is the only behavior change in an otherwise pure alignment/padding PR, it isn't mentioned in the description or any test step, and none of the alignment work depends on it. The 10px threshold was a deliberate dead zone so the narrow Spend header ignores small scrolls near the top; dropping it makes the header start collapsing on the first pixel. That is a scroll-feel change that deserves its own PR and your sign-off, not a silent ride-along here.
Say the word and I'll revert the hunk (restore the constant and the > 10 gate) and leave the rest of the PR as is.
Two corrections to my earlier reply in this thread: the description does not have a "Scroll behavior on Spend" section — I said it did, and it doesn't. And usePreventScrollOnKeyboardInteraction has nothing to do with this line; that was a wrong turn on my part.
What the two versions do
main (index.tsx:51, :133) holds the header still for the first 10px of downward scroll, then tracks the content. Because topBarOffset accumulates per-frame deltas and is clamped to minimalTopBarOffset, the header still collapses fully — it just needs ~10px more scrolling to get there.
This PR moves the header from the first pixel. > 0 still excludes rubber-band overscroll, but nothing filters small or jittery scroll near the top anymore.
If we do decide we want first-pixel tracking, gating on the size of distanceScrolled rather than the absolute offset would keep the noise filtering without costing the first 10px — but again, separate PR.
I could not verify this in a browser on this run (web bring-up failed), so the above is from the diff and the code, not from watching the header move.
There was a problem hiding this comment.
Only by hand, side by side against main — there is no automated coverage for it and it is awkward to add.
The one step that actually distinguishes the change (everything else in the PR's test steps passes either way): on a narrow layout, sit at the top of the Spend list and scroll down by just a few pixels — one trackpad nudge or one arrow-key press. On main the tab row and search input hold still until you pass 10px; with this change they start moving on the first pixel. You have to A/B it against main to see it; in isolation both look fine.
Why there's no test. tests/ui/SearchPageTest.tsx has no scroll assertions at all, and the handler is a Reanimated worklet (index.tsx:117-148) writing to a shared value, so there is nothing Jest can read without mocking the whole Reanimated surface.
So my recommendation from above stands: revert it. A behavior change that needs a main checkout and a 5px trackpad nudge to observe, has no test, and isn't needed for the alignment work is not worth carrying in this PR. Say the word and I'll drop the hunk.
If you'd rather keep it — the full manual pass, and how to make it testable
Manual, narrow layout, Spend page with enough rows to scroll:
- Small-nudge A/B (above) against
main. This is the only step that shows the difference. - Full collapse — scroll down normally. Header collapses fully, no strip left behind. Repeat with filters showing, which swaps the clamp floor to
minimalTopBarWithFiltersOffset. - Restore — scroll back up. Header animates back and the first row is unclipped.
- Overscroll — on mWeb Safari, rubber-band past the top and release. The
> 0guard should keep the header still on the negative offsets; watch for jitter, since this is the case the old 10px dead zone was absorbing. - Focus reset — scroll partway down, then tap the search input. The header snaps back open via
onFocus. - Short list — a list shorter than the viewport. The early return pins the header open; it should never move.
To get real coverage, the offset math would have to come out of the worklet into a pure helper — inputs currentOffset, previousOffset, hasFilterBars, defaultOffset; output the next topBarOffset. That is unit-testable in Jest and would pin both the threshold and the clamp floors. It is a bigger refactor than this PR should carry.
I could not drive this in a browser on this run (web bring-up failed), so the above is from reading the handler, not from watching the header move.
There was a problem hiding this comment.
Lemme know what you think Georgia. I think why I removed this was because as you scrolled down, the tabs/search didn't move with the content until after the 10px. I wanted them to move immediately when you started to scroll.
There was a problem hiding this comment.
Screen.Recording.2026-09-14.at.14.46.11.mov
The behavior after removing TOO_CLOSE_TO_TOP_DISTANCE.
|
@grgia @shawnborton Is this PR not ready for me to review yet? |
joekaufmanexpensify
left a comment
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Change is good for product.
|
@MelvinBot could you sync with main? |
…abAlignment # Conflicts: # src/pages/Search/SearchPageNarrow/index.tsx
|
Pushed a small change to make the gap above the Spend table the same as the gap above the Workspaces table. All good on my end now 👍 |
| inputStyle={[styles.w100, styles.lineHeightUndefined, shouldUseNarrowLayout ? undefined : styles.fontSizeLabel]} | ||
| touchableInputWrapperStyle={shouldUseNarrowLayout ? styles.searchPageInputNarrowTouchableWrapper : styles.searchPageInputWideTouchableWrapper} | ||
| textInputContainerStyles={shouldUseNarrowLayout ? [styles.border, styles.borderRadiusComponentNormal, styles.appBG, styles.p2] : [styles.pb0, styles.ph2]} | ||
| inputStyle={shouldUseNarrowLayout ? [styles.w100, styles.textLabel] : [styles.w100, styles.lineHeightUndefined, styles.fontSizeLabel]} |
There was a problem hiding this comment.
NAB. should we pull [styles.border, styles.borderRadiusComponentNormal, styles.appBG, styles.p2] into one shared style now that TableSearchBar, this input and StaticSearchPageInput all share so they dont drift?
There was a problem hiding this comment.
Totally open to that, your call though!
huult
left a comment
huult
left a comment
There was a problem hiding this comment.
LGTM.
Screen.Recording.2026-09-14.at.16.08.25.mov
|
All you @Gonals, gracias |
|
Are we good to merge this? |
|
🚧 Gonals 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/Gonals in version: 9.4.80-0 🚀
|
|
🤖 No help site changes are required for this PR. I did not create a docs PR. Why: every change here is visual layout — padding, icon presence, and input sizing. Nothing changes a feature name, a tab name, a button label, a setting, or a documented workflow. What I checkedThe diff, change by change:
The articles that describe these screens:
HELP_AUTHORING_GUIDELINES.md requires each article to solve one primary workflow using exact UI terminology. A padding or icon change touches neither, so documenting it would add noise without helping anyone complete a task. There is no help site PR to link, since nothing needs to change. @shawnborton, if you disagree, reply with |
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.4.80-0 🚀
Bundle Size Analysis (Sentry): |
7 participants
Explanation of Change
The tab bars, their top padding, and the search box did not line up across Home/Inbox, Spend, and Workspaces on mobile. This PR aligns them.
Top padding above the tabs
styles.pt1) on theInboxTabSelectorwrapper.SearchTypeMenuNarrowContentwrapper. This covers both consumers, the live tab row and the static variant inStaticSearchTypeMenu.I did not add the padding to the shared
tabSelectorContentContainerstyle. That style also feedsWorkspaceListLayout,TabSelector, andWorkspaceWorkflowsPageRevamp, so a change there would shift those too.Icons in the Inbox tabs on narrow screens
The Inbox tabs now carry icons on narrow screens, matching the other pages. All uses
Feed, Unread usesChatBubbleUnread, and To-do usesTask. Wide screens keep text-only tabs.TabSelectorBaseItemalready accepted an optionalicon, so no shared component changed.Search input on Spend
On narrow screens the Spend search input now matches the search input above the Workspaces table (
TableSearchBar). Three things differed:Border width, radius, color, background, and the green focus border already matched. I updated the static twin in
StaticSearchPageInputalongside the live input so the placeholder does not jump when the real input swaps in.I left out
mnw200(min-width 200), whichTableSearchBaralso sets. On Spend the input isflex1in a row next to the actions bar insidemh5margins, so a 200px floor could overflow on extra-small screens.Padding above the Spend search input row
The row that wraps the search input and the action icons went from 4px to 8px of top padding.
Header height constants
Four constants in
variables.tsencode the exact pixel height of the narrow Spend header, which isposition: absolute:searchListContentMarginTopandsearchListContentWithFiltersMarginTopkeep the list clear of the header.minimalTopBarOffsetandminimalTopBarWithFiltersOffsetare the clamp floor for the collapse animation.The old value of 118 was exactly the old header: tab row 40 + 12, plus input row 4 + 46 + 16. The changes above make the header 124: tab row 4 + 40 + 12, plus input row 8 + 44 + 16. I raised all four by 6. Without this the first list row sits behind the header, and the header leaves a 6px strip visible when collapsed.
I also removed
searchPageInputNarrowTouchableWrapper, which lost its last reference in this PR.Fixed Issues
$ #100880
PROPOSAL:
Tests
Offline tests
These are style and layout changes and do not depend on network state.
QA Steps
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
CleanShot.2026-09-10.at.22.27.14.mp4
MacOS: Chrome / Safari
Inbox, narrow (375x812) — 4px above the tab row, icons on All / Unread / To-do
Inbox, wide (1440x900) — same 4px, text-only tabs
Spend, narrow — 4px above the tab row, 8px above the search + actions row
Spend vs. Workspaces search input — corner, border, radius, text size
Focus state — both borders are RGB(3, 212, 124)
Spend list at the top — first row fully below the header