-
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
Fix/28098: Clicking back button bring back the workspace #28573
Fix/28098: Clicking back button bring back the workspace #28573
Conversation
src/libs/actions/App.js
Outdated
}) | ||
.then(() => Navigation.isNavigationReady()) |
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.
}) | |
.then(() => Navigation.isNavigationReady()) | |
return Navigation.isNavigationReady(); | |
}) |
Little simplification.
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.
just updated. Please help review
This comment was marked as outdated.
This comment was marked as outdated.
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.
Looks good to me.
Edit: We have a bug that would be fixed before moving foraward
@@ -325,9 +325,11 @@ function createWorkspaceAndNavigateToIt(policyOwnerEmail = '', makeMeAdmin = fal | |||
} | |||
|
|||
if (shouldNavigateToAdminChat) { | |||
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(adminsChatReportID)); |
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.
@DylanDylann Why did we remove this line? on staging when we create a new workspace , it should be redirected to admins room.
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.
The dismissModal
should redirect to #admins room but that doesn’t seem to be the case, can you please check?
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.
I replace it by Navigation.dismissModal(adminsChatReportID);
, it still redirects to the admins room, and removes the current RHN modal as well
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.
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:
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.
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?
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.
the browser
s history is just
[/r/{reportID}, /settings, /settings/workspaces]`
That's the expected behavior, no? I mean what's the issue here?
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.
or sorry. My bad. I have updated this comment #28573 (comment)
the browser`s history is just [/r/{reportID}, /settings/workspaces]
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.
@fedirjh do you have any suggestions for this one? The issue is that window.history does not sync with the react-navigation stacks
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.
@fedirjh based on the issue here #28573 (comment), I just updated the PR to fix the error mentioned here #28573 (comment)
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.
Result
Screencast.from.06-10-2023.22.40.21.webm
Screencast.from.06-10-2023.22.39.00.webm
…ng-back-the-worskspace-setting
@mountiny I'm going OOO for the next two weeks, if this is ready to merge, can you please do the last review? Thanks! |
@DylanDylann could you please sync with main and since #16935 is already merged? This simplified it as we are not navigating to additional report first now |
…ng-back-the-worskspace-setting
@mountiny In the staging: https://staging.new.expensify.com/
Screencast.from.20-10-2023.15.43.24.webm
|
@DylanDylann I am still able to reproduce it. Staging CleanShot.2023-10-23.at.02.19.14.mp4 |
@fedirjh Just updated PR to fix the issue: use Screencast.from.23-10-2023.10.54.13.webm |
That’s perfect, @DylanDylann Let's update all other parts where |
@fedirjh please help review the latest change in the PR |
Bug?: Creating a workspace from old dot does not open the #admins room in new dot. I am sure this will be flagged as deploy blocker.
Current PR behavior :CleanShot.2023-10-25.at.11.20.38.mp4Production behavior :CleanShot.2023-10-25.at.11.22.29.mp4 |
@fedirjh I am using the |
I think we can remove it there too so this new behaviour would be expected @fedirjh @DylanDylann |
conflicts |
…e-worskspace-setting
@mountiny fixed conflict |
Reviewer Checklist
Screenshots/VideosWebCleanShot.2023-10-26.at.12.36.17.mp4CleanShot.2023-10-26.at.13.12.13.mp4Mobile Web - ChromeCleanShot.2023-10-02.at.16.20.39.mp4Mobile Web - SafariCleanShot.2023-10-26.at.13.23.46.mp4DesktopCleanShot.2023-10-26.at.13.18.57.mp4iOSCleanShot.2023-10-02.at.16.32.39.mp4AndroidCleanShot.2023-10-02.at.16.16.50.mp4 |
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.
Looks good to me.
@mountiny please help review this one as C+ is approved |
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.
Thank you! @danieldoglas all yours
@danieldoglas please help reivew this PR when you have a chance |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
The performance regression was most likely a false positive, that can be ignored! (Discussion here: https://expensify.slack.com/archives/C05LX9D6E07/p1698667351323659) |
🚀 Deployed to staging by https://github.com/danieldoglas in version: 1.3.94-0 🚀
|
1 similar comment
🚀 Deployed to staging by https://github.com/danieldoglas in version: 1.3.94-0 🚀
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.94-2 🚀
|
🚀 Deployed to staging by https://github.com/danieldoglas in version: 1.3.95-0 🚀
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.95-9 🚀
|
Details
Web - Clicking the back button brings back the workspace settings panel
Fixed Issues
$ #28098
PROPOSAL: #28098 (comment)
Tests
Offline tests
QA Steps
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
Screencast.from.02-10-2023.09.51.33.webm
Mobile Web - Chrome
384648828_6348671445262287_6088025575719771248_n.mp4
Mobile Web - Safari
bring-back-safari.mp4
Desktop
bring-back-desktop-output.mov
iOS
bring-back-ios.mov
Android
Screencast.from.02-10-2023.10.47.24.webm