-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Search v1] Add ReportScreen to RHP #40570
[Search v1] Add ReportScreen to RHP #40570
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍 it's actually less code than I expected.
I tested locally but clicking through 2-3 chat reports and money reports and it seems to work well.
I left you some rename comments which I think will help to make the code more clear
src/styles/index.ts
Outdated
({ | ||
horizontal: 18 + variables.sideBarWidth, | ||
horizontal: shouldUseNarrowLayout ? windowWidth - (variables.sideBarWidth - 18) : 18 + variables.sideBarWidth, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you mind extracting windowWidth - (variables.sideBarWidth - 18)
to a separate variable to give it a meaningful name? To me it's not obvious why this is counted differently for shouldUseNarrowLayout
src/components/MoneyReportHeader.tsx
Outdated
@@ -185,7 +197,7 @@ function MoneyReportHeader({session, policy, chatReport, nextStep, report: money | |||
shouldShowPinButton={false} | |||
report={moneyRequestReport} | |||
policy={policy} | |||
shouldShowBackButton={isSmallScreenWidth} | |||
shouldShowBackButton={shouldUseNarrowLayout || isSmallScreenWidth} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that in general the name shouldUseNarrowLayout
is fitting, but I wanted to suggest a change.
When I look at this line it's clear to me that isNarrowLayout
would be a better name.
In here, right next to each other we have shouldXXX || isYYY
but both refer to the context of the screen/container and its sizing.
So I think if we're passing down layout related flags the names with isXXX
are better and simpler.
src/pages/home/HeaderView.tsx
Outdated
@@ -334,8 +348,10 @@ function HeaderView({report, personalDetails, parentReport, parentReportAction, | |||
)} | |||
</PressableWithoutFeedback> | |||
<View style={[styles.reportOptions, styles.flexRow, styles.alignItemsCenter]}> | |||
{isTaskReport && !isSmallScreenWidth && ReportUtils.isOpenTaskReport(report, parentReportAction) && <TaskHeaderActionButton report={report} />} | |||
{canJoin && !isSmallScreenWidth && joinButton} | |||
{isTaskReport && !(isSmallScreenWidth || shouldUseNarrowLayout) && ReportUtils.isOpenTaskReport(report, parentReportAction) && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these inline checks are so long and so convoluted. Please extract to separate variable before the return
, so that JSX stays cleaner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey lookes even better now, good job 👍
I left some small comments but Im approving as I don't see anything blocking.
You can fix those later
src/hooks/useIsNarrowLayout.ts
Outdated
import useWindowDimensions from './useWindowDimensions'; | ||
|
||
// This hook checks whether narrow layout styles should be applied to the screen. | ||
// SCREENS.SEARCH.REPORT is the screen displaying the chat in RHP, this page should be styled like a page displayed on a small screen. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super nitpicky but this style of comment is nicer for long comments 😅
/** .........
*/
src/pages/home/ReportScreen.tsx
Outdated
@@ -159,6 +160,10 @@ function ReportScreen({ | |||
const flatListRef = useRef<FlatList>(null); | |||
const reactionListRef = useRef<ReactionListRef>(null); | |||
const {isOffline} = useNetwork(); | |||
const activeRoute = useNavigationState(getTopmostRouteName); | |||
const isReportOpenedInRHP = activeRoute === SCREENS.SEARCH.REPORT; | |||
const isNarrowLayout = isSmallScreenWidth || isReportOpenedInRHP; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this use the hook?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I decided to extract the logic from the hook, because one condition below relies on isReportOpenedInRHP variable
src/SCREENS.ts
Outdated
@@ -25,6 +25,7 @@ const SCREENS = { | |||
WORKSPACES_CENTRAL_PANE: 'WorkspacesCentralPane', | |||
SEARCH: { | |||
CENTRAL_PANE: 'Search_Central_Pane', | |||
REPORT: 'Search_Report', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'Search_Report' value is used two times here
@@ -195,6 +195,10 @@ function getAdaptedState(state: PartialState<NavigationState<RootStackParamList> | |||
if (matchingRootRoute?.name === NAVIGATORS.FULL_SCREEN_NAVIGATOR) { | |||
routes.push(createCentralPaneNavigator({name: SCREENS.SETTINGS.WORKSPACES})); | |||
} | |||
if (matchingRootRoute?.state?.routes[0]?.name === SCREENS.SEARCH.CENTRAL_PANE) { | |||
const centralPaneRoute = matchingRootRoute?.state?.routes[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the logic responsible for excluding params for this screen should be in the getMatchingCentralPaneRouteForState
? Instead of removing params we would simply not include them in the first place
@hoangzinh 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] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good overall. Noticed a few bugs while testing, since this is not used in App yet, we don't need to block on them, but it's something we'll need to address once we implement the navigation to this screen
Bug 1: Navigation is not contained in the RHP
bug1.mov
bug4.mov
Bug 2: Back button on not found screen doesn't navigate back
bug2.mov
Bug 3: Back navigation returns to RHP report. Backing up twice leads to an incorrect route
bug3.mov
@WojtekBoman let me know if you wanna address those as part of this PR or in a follow up.
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
I believe bugs related to navigating back can be addressed as follow-ups. However we'd like to discuss the problem of nested navigation in ReportScreen displayed in RHP (bug no.1). Please note that currently there is a multitude of things that can be accessed and clicked from within Report Screen. You can click a mention, room, any link, IOUs, images etc. If we now want to make all this navigation behavior to also works from inside RHP then this is a bigger task. It will require making sure every navigation action makes sense to stay within the RHP and test all the edge cases. This could take much more time, since there might be things we didn't think through. So for now we suggest to stick to displaying only first ReportScreen in RHP in v1 and for future research if we are 100% sure that moving all the navigation to RHP is the way to go. I've consulted it with Mateusz and Adam :) |
Yea, I agree we can address the RHP navigation as a separate issue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. These bugs are still present and will need to be addressed in a follow up, but I think we should focus on the core functionality now given the tight deadline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB:
src/libs/Navigation/Navigation.ts
Outdated
@@ -60,6 +61,8 @@ const dismissModal = (reportID?: string, ref = navigationRef) => { | |||
const report = getReport(reportID); | |||
originalDismissModalWithReport({reportID, ...report}, ref); | |||
}; | |||
// Re-exporting the closeRHPFlow here to fill in default value for navigationRef. The dismissModal isn't defined in this file to avoid cyclic dependencies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Re-exporting the closeRHPFlow here to fill in default value for navigationRef. The dismissModal isn't defined in this file to avoid cyclic dependencies. | |
// Re-exporting the closeRHPFlow here to fill in default value for navigationRef. |
src/libs/Navigation/closeRHPFlow.ts
Outdated
import type {RootStackParamList} from './types'; | ||
|
||
/** | ||
* Closes the last RHP flow, if there is only one, it closes the entire navigator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it closes the entire navigator
I feel this is not correct. It should be something like "if there is only one route in the last RHP stack, it closes that entire stack" not the entire navigator. But I may not understand thoroughly your mean here.
Reviewer Checklist
Screenshots/VideosiOS: Native |
@WojtekBoman I found a weird bug that I couldn't reproduce in neither the latest main branch nor staging but our PR. I haven't found the root cause yet so just post here first Bug 5: Composer's menu is displayed on the right
Screen.Recording.2024-04-24.at.23.27.43.mov |
@WojtekBoman @hoangzinh if fixing this this bug proves to be time consuming, maybe we can tackle it as a follow up. |
@luacmartins It will affect current users if they use a direct link to create a task or any other direct links that open RHP so I'm unsure if we can leave it in a follow-up PR. But it's totally up to you @luacmartins. Screen.Recording.2024-04-25.at.09.04.11.mov |
@hoangzinh I've fixed issues that you mentioned above, could you recheck it? :) |
It works 🚀 @WojtekBoman |
Nice! I'll review this again in a couple of hours. We're getting close! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I have a problem when build IOS native local so I can't screenshot on IOS native but I think we're good.
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/luacmartins in version: 1.4.67-0 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.4.67-7 🚀
|
1 similar comment
🚀 Deployed to production by https://github.com/francoisl in version: 1.4.67-7 🚀
|
@@ -209,7 +221,7 @@ function MoneyRequestHeader({session, parentReport, report, parentReportAction, | |||
cancelText={translate('common.cancel')} | |||
danger | |||
/> | |||
{isSmallScreenWidth && shouldShowHoldMenu && ( | |||
{shouldUseNarrowLayout && shouldShowHoldMenu && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ProcessMoneyRequestHoldMenu is to be shown for small screen devices. However, since shouldUseNarrowLayout
is used here, this will also get shown when the held report is opened in RHP thereby resulting in issue #42007
@@ -269,7 +281,7 @@ function MoneyReportHeader({session, policy, chatReport, nextStep, report: money | |||
nonHeldAmount={!ReportUtils.hasOnlyHeldExpenses(moneyRequestReport.reportID) ? nonHeldAmount : undefined} | |||
requestType={requestType} | |||
fullAmount={fullAmount} | |||
isSmallScreenWidth={isSmallScreenWidth} | |||
isSmallScreenWidth={shouldUseNarrowLayout} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing the shouldUseNarrowLayout
value as isSmallScreenWidth
will stretch the confirmation modal in the web. We should not have changed the isSmallScreenWidth
value, as this change caused the issue #45002
Details
This PR adds a possibility to open ReportScreen in RHP. It's a new functionality available in the new search tab. Currently it's available only from via link in format:
/search/:query/view/:reportID
.Fixed Issues
$ #39883
PROPOSAL:
Tests
/r/:id
=>search/all/view/:id
(to test this page on mobile platforms, copy the modified link and paste it in any report, then click on the link).Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2024-04-23.at.18.26.15.mov
MacOS: Desktop
Screen.Recording.2024-04-23.at.18.42.00.mov