-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Block edit assignee and mark as complete simultaneously #39397
Conversation
@mananjadhav 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] |
@tienifr Can we please have videos for all platforms? I know this isn't reproducible on others, especially difficult on Web and Desktop. |
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.
Waiting on screencasts for all platforms. Rest it works well.
@mananjadhav @rlinoz Please hold up. I just found a critical issue when applying App/src/hooks/useWaitForNavigation.ts Lines 6 to 10 in 860998b
For functions that do not, the promise returned by App/src/components/MenuItem.tsx Line 447 in 860998b
This issue only happens when one instance of |
Thanks for notifying @tienifr. |
@tienifr any updates? |
I tried introducing a new prop App/src/components/MenuItem.tsx Lines 411 to 412 in 860998b
But it takes time to find all the related places. |
@mananjadhav I pushed the changes and retested well. Please take a look. |
Thanks I'll check this tomorrow. |
Hey @mananjadhav did you get a chance to check the latest updates here? I will also review this today. |
I reviewed the changes, need to work on the checklist which I'll finish today. |
This comment has been minimized.
This comment has been minimized.
@rlinoz Can you please trigger the adhoc builds again? especially for Android. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
In android native I'm not being able to hide the keyboard when pressing the back button on the description screen, but I think it is not related to this PR.. |
Thanks for the builds. Testing it now. |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid-assignee-single-execution.mp4Android: mWeb Chromemweb-chrome-assignee-single-execution.mp4iOS: Nativeios-assignee-single-execution.mp4iOS: mWeb Safarimweb-safari-assign-single-execution.mp4MacOS: Chrome / Safariweb-assign-single-execution.movMacOS: Desktopdesktop-assign-single-execution.mov |
@mananjadhav I pulled |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
Let me try again. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
Looks like it is working fine in this last version. |
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.
Tests well, thanks
@mananjadhav can you complete the checklist, please? |
@rlinoz Just did. all yours. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
This PR failing in mWeb with the existing fixed issue #38577 |
Reverted the PR because I could reproduce the issue above and it was causing another blocker. |
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.64-6 🚀
|
Details
Tapping assignee then marking the task as complete at the same time navigates to not found page. This PR applies
singleExecution
to those functions.Fixed Issues
$ #38577
PROPOSAL: #38577 (comment)
Tests
Offline tests
NA
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 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.2024-04-02.at.17.37.04-compressed.mov
Android: mWeb Chrome
Screen.Recording.2024-04-02.at.17.38.32-compressed.mov
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop