-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
pop the deleted workspace page from navigation #21045
Conversation
@allroundexperts 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] |
LGTM 🚀 |
@allroundexperts could you complete a checklist on this one! @adamgrzybowski thanks for reviewing |
Sure @mountiny! |
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-06-20.at.12.20.43.AM.movMobile Web - ChromeScreen.Recording.2023-06-20.at.12.29.47.AM.movMobile Web - SafariScreen.Recording.2023-06-20.at.12.29.22.AM.movDesktopScreen.Recording.2023-06-20.at.12.23.18.AM.moviOSScreen.Recording.2023-06-20.at.12.37.57.AM.movAndroidScreen.Recording.2023-06-20.at.12.34.06.AM.mov |
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.
🚀 🚀 🚀
Can we just confirm what the browser back button does after you have deleted the workspace? @honnamkuan @allroundexperts ? |
Screen.Recording.2023-06-20.at.1.27.12.AM.mov |
Legends, thanks! |
Wouldn't calling |
@honnamkuan Could you test that proposal please, I agree this sounds more straightforwards. |
🚀 Deployed to staging by https://github.com/mountiny in version: 1.3.30-0 🚀
|
Happy to test it out, however I am unable to find App/src/libs/Navigation/Navigation.js Lines 217 to 229 in 342d4fd
or in the documentation https://github.com/Expensify/App/blob/main/contributingGuides/NAVIGATION.md Is there something I missed? |
Yeah, we didn't need the straight function replace(route) {
if (!canNavigate('replace', {route})) {
return;
}
navigationRef.current.dispatch(StackActions.replace(route));
} Also, I don't have the option to create new workspace in FAB button. cc @mountiny is my account missing something? |
@WoLewicki Can you see the workspaces in Settings? The button is not showing if you already have created workspace |
Oh ok, after deleting the workspaces I can see the button, thanks. |
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.30-5 🚀
|
@adamgrzybowski , @mountiny , @allroundexperts This PR is ready for review, thanks.
Details
When deleting workspace, pop the deleted workspace page from navigation stack, so that clicking back button does not route to the deleted workspace page link.
Fixed Issues
$ #20622
PROPOSAL: #20622 (comment)
Tests
Pre-requisite: Login with user with no existing workspace.
Offline tests
N/A
QA Steps
Pre-requisite: Login with user with no existing workspace.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)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
Web
Screen.Recording.2023-06-19.at.11.22.57.PM.mov
Mobile Web - Chrome
Screen.Recording.2023-06-19.at.11.18.26.PM.mov
Mobile Web - Safari
Screen.Recording.2023-06-19.at.11.20.55.PM.mov
Desktop
Screen.Recording.2023-06-19.at.11.21.54.PM.mov
iOS
Screen.Recording.2023-06-19.at.11.19.56.PM.mov
Android
Screen.Recording.2023-06-19.at.11.16.14.PM.mov