Move travel upgrade WS confirmation to a screen#74035
Conversation
melvin-bot
bot
requested review from
jayeshmangwani
and removed request for
a team
|
@jayeshmangwani 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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| import ONYXKEYS from '@src/ONYXKEYS'; | ||
|
|
||
| function WorkspaceConfirmationForTravelPage() { | ||
| const [introSelected] = useOnyx(ONYXKEYS.NVP_INTRO_SELECTED, {canBeMissing: true}); |
There was a problem hiding this comment.
❌ PERF-4 (docs)
The onSubmit function is passed as a prop to WorkspaceConfirmationForm but is not memoized. This creates a new function reference on every render, potentially causing unnecessary re-renders of the WorkspaceConfirmationForm component.
Suggested fix:
const onSubmit = useCallback((params: WorkspaceConfirmationSubmitFunctionParams) => {
createDraftWorkspace(introSelected, '', false, params.name, params.policyID, params.currency, params.avatarFile as File);
createWorkspace({
policyOwnerEmail: '',
makeMeAdmin: false,
policyName: params.name,
policyID: params.policyID,
engagementChoice: undefined,
currency: params.currency,
file: params.avatarFile as File,
});
Navigation.goBack();
}, [introSelected]);There was a problem hiding this comment.
react-compiler automatically memoize it
|
|
||
| const onClose = () => { | ||
| setShouldShowConfirmation(false); | ||
| const openWorkspaceConfirmation = () => { |
There was a problem hiding this comment.
❌ PERF-4 (docs)
The openWorkspaceConfirmation function is passed as a prop to UpgradeIntro but is not memoized. This creates a new function reference on every render, potentially causing unnecessary re-renders of the UpgradeIntro component.
Suggested fix:
const openWorkspaceConfirmation = useCallback(() => {
Navigation.navigate(ROUTES.TRAVEL_WORKSPACE_CONFIRMATION);
}, []);There was a problem hiding this comment.
react-compiler automatically memoize it
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid.movAndroid: mWeb Chromemweb-chrome.moviOS: HybridAppiso.moviOS: mWeb Safarimweb-safri.movMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
| const onSubmit = (params: WorkspaceConfirmationSubmitFunctionParams) => { | ||
| createDraftWorkspace(introSelected, '', false, params.name, params.policyID, params.currency, params.avatarFile as File); | ||
| setShouldShowConfirmation(false); | ||
| useEffect(() => { |
There was a problem hiding this comment.
Why do we need a useEffect for this?
There was a problem hiding this comment.
After upgrade, we want to show the upgrade success view. When we still use a modal, we can easily set the upgrade success state from the upgrade modal view callback. But since now we separate it into a separate page, we need a way to know when the workspace is added.
There was a problem hiding this comment.
Won't this component get re-rendered anytime groupPaidPolicies is updated? In which case, we could just have this be logic that happens without an effect?
There was a problem hiding this comment.
True, it'll be re-rendered. But it also allows the component to show the upgrade success view when the workspace is added from another tab/client.
There was a problem hiding this comment.
Doesn't seem worth needing to use a useEffect to me.
There was a problem hiding this comment.
In which case, we could just have this be logic that happens without an effect?
Okay, would you mind explain how do we achieve this?
There was a problem hiding this comment.
Oh sorry. I'm just saying I don't think it's necessary to show the modal in this tab if someone does something in another tab. So if we take that requirement off the table, do we still need the useEffect?
There was a problem hiding this comment.
Yeah, we still need it.
But since now we separate it into a separate page, we need a way to know when the workspace is added.
The separate page in this case is not in a different tab, but the react-navigation pages.
Previously, we only have TravelUpgrade page and the workspace confirmation view is a modal living inside this page.
But now, we move the workspace confirmation view into a separate page, so now we have TravelUpgrade and WorkspaceConfirmation page. So, in TravelUpgrade, we need a way to know when the workspace is added from the WorkspaceConfirmation page.
|
🚀 Deployed to staging by https://github.com/puneetlath in version: 9.2.55-3 🚀
|
|
✋ 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/puneetlath in version: 9.2.57-0 🚀
|
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.2.57-4 🚀
|
4 participants
Explanation of Change
Fixed Issues
$ #73396
PROPOSAL: #73396 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
Prerequisite:
use a private domain account without a workspace
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.mp4
Android: mWeb Chrome
android.mweb.mp4
iOS: Native
ios.mp4
iOS: mWeb Safari
ios.mweb.mp4
MacOS: Chrome / Safari
web.mp4
MacOS: Desktop
desktop.mp4