[TS migration] Migrate 'Report' page to TypeScript #35566
Conversation
# Conflicts: # src/pages/home/report/ReportActionsView.tsx
# Conflicts: # src/libs/actions/IOU.ts # src/pages/home/HeaderView.tsx
| @@ -37,38 +31,36 @@ function FloatingMessageCounter(props) { | |||
| const show = useCallback(() => { | |||
| Animated.spring(translateY, { | |||
| toValue: MARKER_ACTIVE_TRANSLATE_Y, | |||
| duration: 80, | |||
There was a problem hiding this comment.
It was removed, because duration doesn't exist in spring animation config
| return false; | ||
| } | ||
|
|
||
| if (newProps.isComposerFullSize !== oldProps.isComposerFullSize) { |
There was a problem hiding this comment.
isComposerFullSize wasn't used anymore in the component
| if (lodashGet(newProps, 'policy.avatar') !== lodashGet(oldProps, 'policy.avatar')) { | ||
| return false; | ||
| } | ||
|
|
||
| if (lodashGet(newProps, 'policy.name') !== lodashGet(oldProps, 'policy.name')) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
In this component, policy was used to pass it to ReportActionsList. But it wasn't used inside ReportActionsList.
| viewportOffsetTop, | ||
| isComposerFullSize, | ||
| errors, |
There was a problem hiding this comment.
errors were used in hook dependency only, I wasn't able to find the source of this param as well, so I decided to remove it. If you have any thoughts - let me know
| childManagerAccountID: reportAction.childManagerAccountID, | ||
| childMoneyRequestCount: reportAction.childMoneyRequestCount, | ||
| }), | ||
| const action: ReportAction = useMemo( |
There was a problem hiding this comment.
itsnt better to type it as Partial<ReportAction> ?
There was a problem hiding this comment.
It has all of the required ReportAction fields, so maybe it's better to leave it as ReportAction type
There was a problem hiding this comment.
But why do we still need the as ReportAction assertion below?
There was a problem hiding this comment.
Since ReportAction is a type of ReportActionBase & OriginalMessage, where OriginalMessage is a union, TS gives the error without the assertion:
|
I've found a navigation issue during the testing, should be fixed with this PR |
# Conflicts: # src/pages/home/HeaderView.tsx # src/pages/home/ReportScreen.tsx
# Conflicts: # src/components/MoneyReportHeader.tsx
|
Note: I'll be OOO Feb 10 - Feb 18 🌴 |
# Conflicts: # src/libs/OnyxSelectors/reportWithoutHasDraftSelector.ts # src/pages/home/HeaderView.tsx # src/pages/home/ReportScreen.tsx # src/pages/home/report/ReportActionsList.tsx # src/pages/home/report/ReportActionsView.tsx
src/pages/home/report/FloatingMessageCounter/FloatingMessageCounterContainer/types.ts
Outdated
Show resolved
Hide resolved
| childManagerAccountID: reportAction.childManagerAccountID, | ||
| childMoneyRequestCount: reportAction.childMoneyRequestCount, | ||
| }), | ||
| const action: ReportAction = useMemo( |
There was a problem hiding this comment.
But why do we still need the as ReportAction assertion below?
| isLoadingInitialReportActions={props.isLoadingInitialReportActions} | ||
| isLoadingOlderReportActions={props.isLoadingOlderReportActions} | ||
| isLoadingNewerReportActions={props.isLoadingNewerReportActions} | ||
| policy={props.policy} |
There was a problem hiding this comment.
Why was this prop removed?
There was a problem hiding this comment.
Though policy was mentioned in ReportActionsList props it wasn't used there.
# Conflicts: # src/pages/home/HeaderView.tsx
|
@puneetlath It looks like you got assigned to the issue related to this PR, will you be able to take a look at it? |
| @@ -79,6 +80,9 @@ type PersonalDetails = OnyxCommon.OnyxValueWithOfflineFeedback<{ | |||
|
|
|||
| /** Status of the current user from their personal details */ | |||
| status?: Status; | |||
|
|
|||
| /** Chat report with assignee of task */ | |||
| assigneeChatReport?: Report; | |||
There was a problem hiding this comment.
I don't follow why this is part of the PersonalDetails type. When is this used?
There was a problem hiding this comment.
It used in this part of the code, which added in this PR.
So assignee has PersonalDetails type and it tries to access assignee.assigneeChatReport.
let assignee: OnyxTypes.PersonalDetails | EmptyObject = {};
if (email) {
assignee = Object.values(allPersonalDetails).find((value) => value?.login === email) ?? {};
}
Task.createTaskAndNavigate(report.reportID, title, '', assignee?.login ?? '', assignee.accountID, assignee.assigneeChatReport, report.policyID);
But at the same time I've tried to create several tasks this way, and all of the time assignee.assigneeChatReport was undefined, so maybe we can just get rid of it. Though I'm not sure if this PR is a good place for that, cause it's already kind of complex.
There was a problem hiding this comment.
Yeah, agreed, let's not do it here. Would you mind opening a separate issue to clean that up?
There was a problem hiding this comment.
@puneetlath Yeah, I'm okay to do that 👌
# Conflicts: # src/pages/home/report/ReportActionsView.tsx
|
Recordings for additional test steps: Android: NativeANDROID1.mp4Android: mWeb ChromeANDROID_WEB11.mp4ANDROID_WEB21.mp4iOS: NativeIOS_PT1.mp4iOS: mWeb SafariIOS_WEB_PT1.mp4MacOS: Chrome / SafariWEB_PT1.mp4MacOS: DesktopDESKTOP_PT1.mp4 |
# Conflicts: # src/libs/OnyxSelectors/reportWithoutHasDraftSelector.ts
# Conflicts: # src/pages/home/ReportScreen.tsx # src/pages/home/report/ReportActionsList.tsx # src/pages/home/report/ReportActionsView.tsx
|
@puneetlath Conflicts are resolved! |
|
@puneetlath looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
All tests had passed when I merged. |
|
@VickyStash please link the personal details issue here once you've created it |
|
@puneetlath Here is created issue 👌 |
| lastMentionedTime: reportProp.lastMentionedTime, | ||
| (): OnyxTypes.Report => ({ | ||
| lastReadTime: reportProp?.lastReadTime, | ||
| reportID: reportProp?.reportID ?? '', |
There was a problem hiding this comment.
@VickyStash Is this change intentional or just to fix TS error? It's possible that reportID can be undefined.
We're blocked here because of this change.
There was a problem hiding this comment.
@aimane-chnaif It was done to follow TS typing
| const onListLayout = useCallback((e) => { | ||
| setListHeight((prev) => lodashGet(e, 'nativeEvent.layout.height', prev)); | ||
| const onListLayout = useCallback((event: LayoutChangeEvent) => { | ||
| setListHeight((prev) => event.nativeEvent.layout.height ?? prev); |
There was a problem hiding this comment.
Android app crashes on this line
android.mov
There was a problem hiding this comment.
@aimane-chnaif thank you, I'll raise a follow-up PR
|
🚀 Deployed to staging by https://github.com/puneetlath in version: 1.4.51-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.4.51-3 🚀
|
Details
[TS migration] Migrate 'Report' page to TypeScript
Fixed Issues
$ #25216
PROPOSAL: N/A
Tests
Part 1
New Messagesfloat button, tap on it -> the screen should be scrolled down, the button should disappear.Part 2
adminchat. Tap on the description in the header -> description update screen should appear.Offline tests
Part 1 Steps 1-12 and Part 2 from the Tests section.
Make sure it works the same was as before the migration.
QA Steps
Same as in the Tests section.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel so 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
android1.mp4
Android: mWeb Chrome
android_web1.mp4
iOS: Native
ios1.mp4
iOS: mWeb Safari
ios_web_1.mp4
ios_web_2.mp4
MacOS: Chrome / Safari
web1.mp4
MacOS: Desktop
desktop1.mp4