-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Prevent multiple chats from opening when switching quickly #58051
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
Conversation
@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] |
Perf tests are failing. Please try merging main and pushing again. |
👍 |
@huult Do you think adding automated tests here would be helpful? I think we can add one here—mocking the SidebarLinks, simulating the first two presses, and expecting the first button's fire event to be called. |
@jayeshmangwani I believe we don't need a test here because the SidebarLink only displays the report page at home and doesn't allow navigation elsewhere in a narrow layout. The automation test wouldn't bring much value in this case, and setting up the test for the sidebar is quite complex. Let me know your thoughts |
fair enough |
|
||
// Prevent opening a new Report page if the user quickly taps on another conversation | ||
// before the first one is displayed. | ||
const shouldOpenReportInNarrowLayout = Navigation.getActiveRoute() !== '/home' && shouldUseNarrowLayout; |
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.
@huult Sorry for the previous confusion, but this var name I suggested actually contradicts what we do here. I'm not great at naming variables, but it should be something that clearly defines its purpose. I'm not sure what the best would be, but shouldBlockReportNavigation
comes to mind. What do you think, @youssef-lr ?
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid.movAndroid: mWeb Chromemweb-chrome.moviOS: NativeiOS.moviOS: mWeb Safarimweb-safari.movMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
@huult Code changes look good! I just have a comment about the variable name (mentioned above). However, I'm not sure why the perf-tests are failing on our PR - I checked a few other PRs, and their tests aren't failing. |
@jayeshmangwani I fixed the perf-test and updated the variable name. Could you check it 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.
LGTM 🚀
✋ 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/youssef-lr in version: 9.1.13-0 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.1.13-5 🚀
|
Details
Fixed Issues
$ #56947
PROPOSAL: #56947 (comment)
Tests
Same QA step
Offline tests
QA Steps
Prerequisite: Account should have at least two different conversations on LHN
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.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
Android: Native
Screen.Recording.2025-03-08.at.21.40.39.mp4
Android: mWeb Chrome
Screen.Recording.2025-03-08.at.21.42.25.mp4
iOS: Native
Screen.Recording.2025-03-08.at.21.44.43.mp4
iOS: mWeb Safari
Screen.Recording.2025-03-08.at.21.45.18.mp4
MacOS: Chrome / Safari
Screen.Recording.2025-03-08.at.21.46.17.mp4
MacOS: Desktop
Screen.Recording.2025-03-08.at.21.47.31.mp4