-
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
fix/28562: text input in the Composer disappears #32799
fix/28562: text input in the Composer disappears #32799
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
Build is in progress ... |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
@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] |
Oops, i made this open befoe the Author checklist was added. My bad |
@rinej could you please go ahead and prepare this for review? |
@Julesssss Sure, I am on it |
@Julesssss the PR is ready for the review |
// Adding isWeb check to fix issue on Android, assuming we don't need tab sync on mobiles - https://github.com/Expensify/App/issues/28562 | ||
const shouldSyncComment = isWeb && prevCommentProp !== comment && value !== 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.
Ah sorry for not mentioning this earlier, but ideally we don't use getPlatform
and instead create a specific file for the web-specific code.
Reviewer Checklist
Screenshots/VideosAndroid: NativeUploading Screen Recording 2023-12-22 at 9.54.51 AM.mov… Android: mWeb ChromeScreen.Recording.2023-12-22.at.9.52.48.AM.moviOS: NativeScreen.Recording.2023-12-22.at.9.52.03.AM.moviOS: mWeb SafariScreen.Recording.2023-12-22.at.9.51.35.AM.movMacOS: Chrome / SafariScreen.Recording.2023-12-22.at.9.44.20.AM.movMacOS: DesktopScreen.Recording.2023-12-22.at.9.47.09.AM.mov |
Lint errors @rinej! |
@Julesssss I updated the PR and created specific file for android, so we do not update the comment field when the prev value is different. With that we disable the tab synchronization on android and it fixes the current bug. @allroundexperts I also fixed the lint errors Without the fix: After the fix: |
@@ -52,6 +52,7 @@ function SilentCommentUpdater({comment, commentRef, report, value, updateComment | |||
useEffect(() => { | |||
// Value state does not have the same value as comment props when the comment gets changed from another tab. | |||
// In this case, we should synchronize the value between tabs. | |||
|
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.
Please remove unnecessary newline
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.
done
Bumping @allroundexperts for C+ review |
On it today @Julesssss! |
@Julesssss I noticed that the PR Author Checklist failed, even though I have checked all checkboxes. Should I add some additional ones? |
Hmm, maybe the checklist was just updated... We can ignore |
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!
@Julesssss how can we move forward with the PR? |
I was awaiting final C+ approval, then was OOO. Approving now |
✋ 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/Julesssss in version: 1.4.22-0 🚀
|
🚀 Deployed to staging by https://github.com/Julesssss in version: 1.4.22-0 🚀
|
🚀 Deployed to production by https://github.com/thienlnam in version: 1.4.22-6 🚀
|
Details
Fixed Issues
$ #28562
PROPOSAL:
$ #28562 (comment)
Tests
Same as QA
Offline tests
QA Steps
It should be tested on high traffic account and on android device
In order to simulate low performing device we can change the debounce time as stated here:
#28562 (comment)
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)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label 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
android.mov
Android: mWeb Chrome
android-web2.mp4
iOS: Native
iOS.mp4
iOS: mWeb Safari
iOS-web.mp4
MacOS: Chrome / Safari
WEB.mp4
MacOS: Desktop
desktop.mp4