Fix: Exports list is not scrollable when export options exceed viewport height#85104
Fix: Exports list is not scrollable when export options exceed viewport height#85104nyomanjyotisa wants to merge 4 commits intoExpensify:mainfrom
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
melvin-bot
bot
requested review from
heyjennahay
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/components/MoneyReportHeader.tsx
Outdated
| }, [originalSelectedTransactionsOptions, showDeleteModal, dismissedRejectUseExplanation, isDelegateAccessRestricted, showDelegateNoAccessModal]); | ||
|
|
||
| const shouldShowSelectedTransactionsButton = !!selectedTransactionsOptions.length && !transactionThreadReportID; | ||
| const shouldPopoverUseScrollView = |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The shouldPopoverUseScrollView computation logic is now duplicated in 4 places across the codebase: SettlementButton/index.tsx, SearchBulkActionsButton.tsx, MoneyReportHeader.tsx, and MoneyRequestReportActionsList.tsx. All four perform the identical pattern:
options.length >= CONST.DROPDOWN_SCROLL_THRESHOLD ||
options.some((option) => (option.subMenuItems?.length ?? 0) >= CONST.DROPDOWN_SCROLL_THRESHOLD);Extract this into a shared utility function, for example:
// src/libs/shouldPopoverUseScrollView.ts
import CONST from '@src/CONST';
import type {DropdownOption} from '@components/ButtonWithDropdownMenu/types';
function shouldPopoverUseScrollView<T>(options: Array<DropdownOption<T>>): boolean {
return (
options.length >= CONST.DROPDOWN_SCROLL_THRESHOLD ||
options.some((option) => (option.subMenuItems?.length ?? 0) >= CONST.DROPDOWN_SCROLL_THRESHOLD)
);
}
export default shouldPopoverUseScrollView;Then use it in all four call sites:
const popoverUseScrollView = shouldPopoverUseScrollView(selectedTransactionsOptions);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| }); | ||
| }, [originalSelectedTransactionsOptions, dismissedRejectUseExplanation, isDelegateAccessRestricted, showDelegateNoAccessModal]); | ||
|
|
||
| const shouldPopoverUseScrollView = |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
Same duplicated shouldPopoverUseScrollView computation as in MoneyReportHeader.tsx, SettlementButton/index.tsx, and SearchBulkActionsButton.tsx. This should use the shared utility function suggested in the other comment to eliminate the 4-way duplication.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
@nyomanjyotisa I think following DRY principle makes sense, wdyt?
There was a problem hiding this comment.
Oh yeah, sorry I missed the bot comment. I will update the PR
eh2077
left a comment
eh2077
left a comment
There was a problem hiding this comment.
It's better to add unit tests as well.
| }); | ||
| }, [originalSelectedTransactionsOptions, dismissedRejectUseExplanation, isDelegateAccessRestricted, showDelegateNoAccessModal]); | ||
|
|
||
| const shouldPopoverUseScrollView = |
There was a problem hiding this comment.
@nyomanjyotisa I think following DRY principle makes sense, wdyt?
2 participants
Explanation of Change
Add
shouldPopoverUseScrollViewtoButtonWithDropdownMenuinMoneyReportHeaderandMoneyRequestReportActionsListto enable scrolling when any option has submenu items that exceed the thresholdFixed Issues
$ #84757
PROPOSAL: #84757 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
Precondotion:
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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-Native.mp4
Android: mWeb Chrome
Android-mWeb.Chrome.mp4
iOS: Native
iOS-Native.mp4
iOS: mWeb Safari
iOS-mWeb.Safari.mp4
MacOS: Chrome / Safari
MacOS-Chrome.mp4