Use a larger button for multi-select on web#88146
Conversation
|
🚧 @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.
…ctions button Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
🚧 @shawnborton 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! 🧪🧪
|
|
Note that I did get this feedback:
But I actually feel like the negative margin trick is the most simple, straightforward way to handle this without needing to reconfigure those filter wrappers. |
|
@MelvinBot can you help me fill out the PR author checklist please? |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
@thelullabyy Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
melvin-bot
bot
requested review from
trjExpensify
and removed request for
a team
|
@shawnborton I'm reviewer of this PR, could you please add me as assignee so I can talk directly with Melvin for updates. Thx |
|
cc @luacmartins @JS00001 @Expensify/design @JmillsExpensify |
|
I will handle the updates with Melvin as the PR author, you can stay on as a reviewer though. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| searchBulkActionsButton: { | ||
| marginVertical: 3, | ||
| marginVertical: -3, |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
The value -3 is a magic number with no explanation of why this specific value is used. Negative margins are especially worth documenting since they are an unusual layout technique that can cause clipping or overlap issues. The previous value 3 was also a magic number, but switching to a negative margin makes the intent even less clear.
Define a named constant or add a comment explaining why -3 is the correct value. For example:
// Negative margin compensates for the larger default button size
// to keep the bulk actions bar at the same height
const SEARCH_BULK_ACTIONS_BUTTON_VERTICAL_OFFSET = -3;
searchBulkActionsButton: {
marginVertical: SEARCH_BULK_ACTIONS_BUTTON_VERTICAL_OFFSET,
},Reviewed at: ed27170 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
@MelvinBot can you implement that suggestion?
Can you also explain in that comment that we arrived at that number because the current height of the view is 34. The bigger button is 40, which is 6 more. That means we need to pull the button up by half of the difference, so 6/2 is 3.
There was a problem hiding this comment.
@MelvinBot you are an assignee now, please retry this request
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed2717099b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ) : ( | ||
| <View style={[styles.flexRow, styles.gap3]}> | ||
| <View style={[styles.flexRow, styles.alignItemsCenter, styles.gap3]}> | ||
| <ButtonWithDropdownMenu |
There was a problem hiding this comment.
Removing buttonSize={CONST.DROPDOWN_BUTTON_SIZE.SMALL} here makes ButtonWithDropdownMenu fall back to MEDIUM for every wide-layout client, not just web. SearchPage chooses wide vs narrow using shouldUseNarrowLayout (width-based), so iOS/Android tablets also execute this path and get the larger control (plus the accompanying spacing offset in styles.searchBulkActionsButton), which conflicts with the stated goal of a web-only visual tweak and can regress native tablet layout consistency.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Sorry I don't mean web-only, I actually mean wide screen only. So I think we're good here.
|
I'd be happy to add that comment explaining the To have me make this fix, add me ( |
|
I've updated the PR body with the filled-out author checklist. All N/A items are checked with explanations. Items left unchecked that need your action:
|
|
@MelvinBot In src/styles/index.ts on line 4904: please define a named constant or add a comment explaining why -3 is the correct value. For example: // Negative margin compensates for the larger default button size searchBulkActionsButton: { Can you also explain in that comment that we arrived at that number because the current height of the view is 34. The bigger button is 40, which is 6 more. That means we need to pull the button up by half of the difference, so 6/2 is 3. |
The filter bar row is 34px and the larger button is 40px, so we offset by half the difference ((40-34)/2 = 3) to prevent layout shift. Co-authored-by: Shawn Borton <[email protected]>
|
Added an inline comment explaining the -3 value: I kept the value inline rather than extracting a named constant since it's only used in this one spot and the comment makes the reasoning clear. Let me know if you'd still prefer the constant approach. |
|
@thelullabyy feel free to start final review |
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
Looks good to me.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
@MelvinBot You completed 43 out of 50 checklist items with 7 unfinished items in PR author checklist. Please finish |
|
The remaining 7 unchecked items require action from the PR author — I can't check them off automatically. Here's a breakdown: Can likely be checked off by
Needs action:
|
shawnborton
changed the title
|
Updated the checklist and the PR name. |
5 participants
Explanation of Change
This makes the "X selected" button for bulk editing use a larger size:
CleanShot.2026-04-17.at.09.16.14.mp4
Fixed Issues
$ #88176
PROPOSAL: N/A - fixed by internal Designgineer
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick) — N/A, no callbacks changedsrc/languages/*files and using the translation method — N/A, no new copySTYLE.md) were followed — N/A, no JSDoc changesAvatar, I verified the components usingAvatarare working as expected) — QA steps cover both expense rows and report rowssearchBulkActionsButtonStyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases) — Changes are scoped toSearchBulkActionsButtononlyDesignlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page. — N/Amainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps. — N/AScreenshots/Videos
MacOS: Chrome / Safari
CleanShot.2026-04-17.at.09.16.14.mp4