Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix/28098: Clicking back button bring back the workspace #28573

Merged
merged 10 commits into from
Oct 30, 2023
Merged

Fix/28098: Clicking back button bring back the workspace #28573

Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a27b365
fix clicking back button bring back the workspace
DylanDylann Oct 2, 2023
a803a71
fix refactor logic then
DylanDylann Oct 2, 2023
4fe4ff0
fix handle case create workspace without openning modal
DylanDylann Oct 2, 2023
ddb0f4c
Merge branch 'Expensify:main' into fix/28098-clicking-back-button-bri…
DylanDylann Oct 4, 2023
392e320
fix wait for browser history load url after navigate
DylanDylann Oct 6, 2023
fd3300d
Merge branch 'Expensify:main' into fix/28098-clicking-back-button-bri…
DylanDylann Oct 20, 2023
a88dabc
fix update the latest change in staging
DylanDylann Oct 23, 2023
54a01bb
fix on press create new workspace
DylanDylann Oct 23, 2023
cd3261a
fix remove createWorkspaceAndNavigateToIt
DylanDylann Oct 23, 2023
2d732e2
Merge branch 'main' into fix/28098-clicking-back-button-bring-back-th…
DylanDylann Oct 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/libs/actions/App.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,9 +325,11 @@ function createWorkspaceAndNavigateToIt(policyOwnerEmail = '', makeMeAdmin = fal
}

if (shouldNavigateToAdminChat) {
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(adminsChatReportID));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DylanDylann Why did we remove this line? on staging when we create a new workspace , it should be redirected to admins room.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dismissModal should redirect to #admins room but that doesn’t seem to be the case, can you please check?

Copy link
Contributor Author

@DylanDylann DylanDylann Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I replace it by Navigation.dismissModal(adminsChatReportID);, it still redirects to the admins room, and removes the current RHN modal as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned in the proposal #28098 (comment):

we can make sure that the current RIGHT_MODAL_NAVIGATOR screen is REPLACE by report screen, rather than PUSH a new report screen to the stack by using Navigation.dismissModal function:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it still redirects to the admins room, and removes the current RHN modal as well

It doesn’t work for me.

Can we just dismiss the modal , navigate to admins then open the modal again?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the browsers history is just [/r/{reportID}, /settings, /settings/workspaces]`

That's the expected behavior, no? I mean what's the issue here?

Copy link
Contributor Author

@DylanDylann DylanDylann Oct 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or sorry. My bad. I have updated this comment #28573 (comment)

the browser`s history is just [/r/{reportID}, /settings/workspaces]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fedirjh do you have any suggestions for this one? The issue is that window.history does not sync with the react-navigation stacks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fedirjh based on the issue here #28573 (comment), I just updated the PR to fix the error mentioned here #28573 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Result

#28573 (comment)

Screencast.from.06-10-2023.22.40.21.webm
Screencast.from.06-10-2023.22.39.00.webm

Navigation.dismissModal(adminsChatReportID);
}

return Navigation.isNavigationReady();
})
.then(() => {
Navigation.navigate(ROUTES.WORKSPACE_INITIAL.getRoute(policyID));
})
.then(endSignOnTransition);
Expand Down
Loading