Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
WojtekBoman
force-pushed
the
swrhp-v2
branch
3 times, most recently
from
052c950 to
17237de
Compare
src/components/MoneyReportHeader.tsx
Outdated
| const {wideRHPRouteKeys, superWideRHPRouteKeys} = useContext(WideRHPContext); | ||
| const [network] = useOnyx(ONYXKEYS.NETWORK, {canBeMissing: true}); | ||
| const shouldDisplayNarrowMoreButton = !shouldDisplayNarrowVersion || (wideRHPRouteKeys.length > 0 && !isSmallScreenWidth); | ||
| const shouldDisplayNarrowMoreButton = !shouldDisplayNarrowVersion || ((wideRHPRouteKeys.length > 0 || superWideRHPRouteKeys.length > 0) && !isSmallScreenWidth); |
There was a problem hiding this comment.
We should switch the implementation here to use the shouldUseNarrowLayout wrapper instead
|
|
||
| const {shouldUseNarrowLayout} = useResponsiveLayout(); | ||
| // eslint-disable-next-line rulesdir/prefer-shouldUseNarrowLayout-instead-of-isSmallScreenWidth | ||
| const {shouldUseNarrowLayout: shouldUseNarrowLayoutByDefault, isSmallScreenWidth} = useResponsiveLayout(); |
| const styles = useThemeStyles(); | ||
| // eslint-disable-next-line rulesdir/prefer-shouldUseNarrowLayout-instead-of-isSmallScreenWidth | ||
| const {isSmallScreenWidth, isMediumScreenWidth, isLargeScreenWidth, shouldUseNarrowLayout} = useResponsiveLayout(); | ||
| const {isSmallScreenWidth, isMediumScreenWidth, isLargeScreenWidth, shouldUseNarrowLayout: shouldUseNarrowLayoutByDefault} = useResponsiveLayout(); |
| const {translate, localeCompare} = useLocalize(); | ||
| // eslint-disable-next-line rulesdir/prefer-shouldUseNarrowLayout-instead-of-isSmallScreenWidth | ||
| const {shouldUseNarrowLayout, isSmallScreenWidth, isMediumScreenWidth} = useResponsiveLayout(); | ||
| const {shouldUseNarrowLayout: shouldUseNarrowLayoutByDefault, isSmallScreenWidth, isMediumScreenWidth} = useResponsiveLayout(); |
| const secondOverlayRHPOnSuperWideRHPProgress = new Animated.Value(0); | ||
| const thirdOverlayProgress = new Animated.Value(0); | ||
|
|
||
| // This array contains the names of wide and super wide right modals. |
There was a problem hiding this comment.
It seems to me it would be better to split this into three screen sets for WRHP:
Wide RHP – search_report
Super-Wide RHP – money_request_report, expense_report
combined - money_request_report, expense_report, search_report
|
|
||
| const lastRHPKeys = extractNavigationKeys(lastRHPRoute.state); | ||
| const superWideRHPIndex = | ||
| lastRHPRoute.state?.routes.findLastIndex((route) => route.name === SCREENS.RIGHT_MODAL.SEARCH_MONEY_REQUEST_REPORT || route.name === SCREENS.RIGHT_MODAL.EXPENSE_REPORT) ?? -1; |
There was a problem hiding this comment.
We can use here SUPER_WIDE_RIGHT_MODALS.has()
| const superWideRHPIndex = | ||
| lastRHPRoute.state?.routes.findLastIndex((route) => route.name === SCREENS.RIGHT_MODAL.SEARCH_MONEY_REQUEST_REPORT || route.name === SCREENS.RIGHT_MODAL.EXPENSE_REPORT) ?? -1; | ||
|
|
||
| const wideRHPIndex = lastRHPRoute.state?.routes.findLastIndex((route) => route.name === SCREENS.RIGHT_MODAL.SEARCH_REPORT) ?? -1; |
There was a problem hiding this comment.
And here we can use WIDE_RIGHT_MODALS.has() (after it only contains one screen) for consistency
| return routeName === SCREENS.RIGHT_MODAL.SEARCH_REPORT; | ||
| } | ||
|
|
||
| function SecondaryOverlay() { |
There was a problem hiding this comment.
I think we can move the SecondaryOverlay component into a separate file located in the same folder as Overlay and BaseOverlay
There was a problem hiding this comment.
For now, I would leave it in this file as it's used only here, it's easier to find it and we don't need to jump between files
| const singleRHPWidth = variables.sideBarWidth; | ||
| const getWideRHPWidth = (windowWidth: number) => variables.sideBarWidth + calculateReceiptPaneRHPWidth(windowWidth); | ||
|
|
||
| function SecondaryOverlay() { |
There was a problem hiding this comment.
We can move this component as well to the same place where we have Overlay and BaseOverlay.
I’d also suggest merging these two components into a single one that returns both overlays at once, for example, as an object with two fields (innerOverlay and outerOverlay), or as a component with a boolean prop like shouldReturnInnerOverlay.
| const screenOptions = useRHPScreenOptions(); | ||
| const {shouldRenderSecondaryOverlay, isWideRHPFocused, shouldRenderTertiaryOverlay, isWideRHPClosing, clearWideRHPKeys, syncWideRHPKeys, syncSuperWideRHPKeys} = | ||
| useContext(WideRHPContext); | ||
| const {shouldRenderTertiaryOverlay, clearWideRHPKeys, syncWideRHPKeys, syncSuperWideRHPKeys} = useContext(WideRHPContext); |
There was a problem hiding this comment.
Maybe we can extract the tertiary overlay into a separate component as well?
| return false; | ||
| } | ||
| const params = lastRoute.params; | ||
| if (params && 'screen' in params && typeof params.screen === 'string' && params.screen === SCREENS.RIGHT_MODAL.SEARCH_REPORT) { |
There was a problem hiding this comment.
Here we also can use WIDE_RIGHT_MODALS.has() after refactor
melvin-bot
bot
requested review from
ZhenjaHorbach and
trjExpensify
and removed request for
a team
|
@ZhenjaHorbach 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] |
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
I created an empty report > created an expense to add to it and then I got this view: 2025-12-05_18-16-14.mp4I can't seem to delete the expense from that view either. |
|
Not sure what to do about this, but the sizing of the modal changing while it's loading is a bit jarring. Even if I'm cycling through reports that should go from super wide > super wider, it still goes to a skeleton loader of the wide modal first: 2025-12-05_18-13-08.mp4 |
|
Yet to confirm, but looks like this PR caused #77606 |
Revert "Merge pull request #75886 from software-mansion-labs/swrhp-v2"
|
This PR is fail in Test 2 with this issue Bug7028707_1765628675164.77578-PR75886_-_Expense_preview_increase_when_navigating_a_single_expense___empty_report_with_arrow.mp4 |
…s/swrhp-v2" This reverts commit e386d63.
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.2.78-8 🚀
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.2.79-0 🚀
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.2.81-0 🚀
|
…s/swrhp-v2" This reverts commit e386d63.
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.2.81-5 🚀
|
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.2.81-5 🚀
|
…s/swrhp-v2" This reverts commit e386d63.
…s/swrhp-v2" This reverts commit e386d63.
…s/swrhp-v2" This reverts commit e386d63.
…s/swrhp-v2" This reverts commit e386d63.
…s/swrhp-v2" This reverts commit e386d63.
…s/swrhp-v2" This reverts commit e386d63.
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.3.3-0 🚀
|
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.3.3-8 🚀
|
…s/swrhp-v2" This reverts commit e386d63.
12 participants
Explanation of Change
This PR adds a new route to display the expense report in the Inbox tab and changes the way the expense report is displayed in the Reports tab from central screen to Super Wide RHP.
In this PR, the SecondaryOverlay has also been moved as a separate component and
secondaryOverlayProgresshas been split into 3 variables depending on the case being handledFixed Issues
$ #71821
PROPOSAL:
Tests
Test 1
Test 2
Test 3
Test 4
Test 5
Test 6
Verify if these bugs don't appear:
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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: mWeb Chrome
iOS: Native
Screen.Recording.2025-12-05.at.14.19.57.mov
iOS: mWeb Safari
Screen.Recording.2025-12-05.at.14.36.06.mov
MacOS: Chrome / Safari
MacOS: Desktop
Screen.Recording.2025-12-05.at.09.34.25.mov