[HOLD for payment 2023-07-24] [Navigation Refactor] Follow-ups from the migration PR

[mountiny]

Part of #11768

We have merged this migration PR after a lot of testing and iterations, but there are still some follow ups which we need to address but we have left it out in sake of urgency since we could wait on such big PR forever.

• FreezeWrapper displayName missing @swm/navigation refactor #19263 (comment)
• functional component CustomRouter should use function @swm/navigation refactor #19263 (comment)
• Same goes for ThreePaneView @swm/navigation refactor #19263 (comment)
• refactor ReportScreenWrapper to a functional component @swm/navigation refactor #19263 (comment)
• Simple comment update @swm/navigation refactor #19263 (comment)
• Make CurrentReportIdContextProvider a functional component
• Explore using a skeleton for loading an app after sign in @swm/navigation refactor #19263 (comment)
• Clarify the question mark in the reportID path @swm/navigation refactor #19263 (comment)
• Check potential duplication of the event listeners @swm/navigation refactor #19263 (comment)
• check if patch for @react-navigation/native is still needed
• fix tests by either another approach for rendering heavy ReportScreen or properly handling navigation in tests
• look deeper at logic for getIsReportFullyVisible since part of it was needed due to drawer being able to cover the report screen
• fix flows for Task in RHP
• Remove close props from HeaderWithBackButton if it is meant not to be used there anymore
• check some weird navigation flows with browser back buttons and RHP
• deprecate one of the same styles @swm/navigation refactor #19263 (comment)

[melvin-bot]

Triggered auto assignment to @alexpensify (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.

[mountiny]

cc @0xmiroslav @WoLewicki @adamgrzybowski

[0xmiros]

No transition animation. @mountiny this is already known but tracked somewhere?

Screen.Recording.2023-06-07.at.7.42.48.AM.mov

[mountiny]

Thants intentional for v1 #20342

[WoLewicki]

commenting

[mountiny]

Checked off the boxes above which should be addressed by the Pr linked above.

[mountiny]

Assigning Rajat here for later payment for his review.

[alexpensify]

Noted!

[mountiny]

One PR is merged, we will have other PRs comming to resolve this one completely

[alexpensify]

Thank you for the update @mountiny!

[alexpensify]

Looks like we are making progress

[mountiny]

I have requested another SWM person to look into this as Adam is focusing on higher priority issues but it would be nice to get these clean up bits off table

[kosmydel]

Hey, I am from Software Mansion, I am going to help Adam with those follow-ups. I am going to start with the "refactor ReportScreenWrapper to a functional component" task and then continue to look at others.

[kosmydel]

Addressing Remove close props from HeaderWithBackButton if it is meant not to be used there anymore issue:

The close button from HeaderWithBackButton is used in i.g. src/components/KeyboardShortcutsModal.js, so we can't remove it safely.

[alexpensify]

Update for the week: The PR shared looks to be moving forward

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.41-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Refactor ReportScreenWrapper to functional component #22260

If no regressions arise, payment will be issued on 2023-07-24. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[kosmydel]

Hey, I investigated this point:

look deeper at logic for getIsReportFullyVisible since part of it was needed due to drawer being able to cover the report screen

From my observations the current implementation of getIsReportFullyVisible is right. We still need to check both:

• Visibility.isVisible() - removing the drawer didn't change here anything - we still need it,
• isFocused - we still need to use it (on mWeb the RHP can cover the report screen).

So we don't have to do any changes.

export default function getIsReportFullyVisible(isFocused) {
    return Visibility.isVisible() && isFocused;
}

[parasharrajat]

Looks correct to me. I think we changed this during other refactors.

[adamgrzybowski]

Hey @mountiny, it looks like we went through all the points from this list.

These two are quite mysterious

• fix flows for Task in RHP
• check some weird navigation flows with browser back buttons and RHP

I talked with @WoLewicki about them and they may have separate issues or be resolved already

Besides, there is the one with tests but we may create a separate issue and work on it when all other more important tasks are done

• fix tests by either another approach for rendering heavy ReportScreen or properly handling navigation in tests

So maybe we could close this issue to clean the board a little

[mountiny]

There will be $1000 payment to @parasharrajat noting.

[parasharrajat]

@mountiny I reviewed two PRs. Should the payment be more in this case like 2K or have you fixed this issue at 1k?

[mountiny]

Lets do $1500 as they were kinda related, and one of them was a functional component refactor

[parasharrajat]

Ok. Thanks. I will request this soon and inform here.

[alexpensify]

@parasharrajat did you send the payment request? If yes, I will close out this GH. Thank you for the update!

[alexpensify]

Here is the payment summary:

• External issue reporter: N/A
• Contributor that fixed the issue: N/A
• Contributor+ that helped on the issue and/or PR: @parasharrajat $1500

Upwork Job: N/A

*If applicable, the bonuses will be applied on the final payment

Extra Notes regarding payment: #20320 (comment)

All set here @JmillsExpensify! This one was pre-summary days.

[alexpensify]

No update, so I'm going to close for now.

[parasharrajat]

Payment requested. Ref: #20320 (comment)

[JmillsExpensify]

Reviewed details for @parasharrajat. These details are accurate based on summary from Business Reviewer and are now approved for payment in NewDot.