Fix for double paste in composer after function component mig#25419
Fix for double paste in composer after function component mig#25419bondydaa merged 1 commit intoExpensify:mainfrom
Conversation
|
|
||
| if (textInput.current) { | ||
| textInput.current.addEventListener('paste', handlePaste); | ||
| document.addEventListener('paste', handlePaste); |
There was a problem hiding this comment.
I know this is original code before function component refactor.
But I think it's better to set textInput as target as long as it doesn't cause any issues.
@bondydaa what do you think?
There was a problem hiding this comment.
yeah I was wondering about using document as well.
my gut says having it on the dom element instead of the document object is probably better, in the event we add other global paste type handlers in the future so it won't create a conflict in that event.
There was a problem hiding this comment.
asked in open source here https://expensify.slack.com/archives/C01GTK53T8Q/p1692292251654389
There was a problem hiding this comment.
Hmmm previously I fixed an error where multiple composers existed at the same time like editing multiple messages and that's why handlePaste haves some logic of visibility. I'm not sure if maybe there is some logic behind that but I would prefer not to mess with it in case it does. Although if we move it to textInput we coudla void that logic of focus.
cc @mountiny in case you know there is logic about this
There was a problem hiding this comment.
FYI Here is the issue of multiple composers #23502
There was a problem hiding this comment.
I think we can go with this solution (document listener) for now as deploy blocker and discussion may not end up very soon.
There was a problem hiding this comment.
👍 thanks for that extra context and yeah agree let's go with this for now, we can always fix it later if needed too. this stuff isn't being carved into stone 😂
There was a problem hiding this comment.
YEah probably that would've been a possible solution for #22803 maybe @Santhosh-Sellavel can give us a clue on why it was fixed in document instead of textInput
There was a problem hiding this comment.
We used document only earlier
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-08-17.at.11.32.38.PM.movMobile Web - ChromeMobile Web - SafariScreen.Recording.2023-08-17.at.11.36.17.PM.movDesktopScreen.Recording.2023-08-17.at.11.37.08.PM.moviOSAndroid |
|
asking to get this CP'd internally 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. |
Fix for double paste in composer after function component mig (cherry picked from commit 8a856e7)
|
🚀 Deployed to staging by https://github.com/bondydaa in version: 1.3.55-3 🚀
|
Details
Side effect introduced in the migration to functional component #23359 just implementing as it was before in the class component.
Fixed Issues
$ #25409
PROPOSAL: #25409 (comment)
Tests
Offline tests
Same as tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
Web.double.paste.-.8_17_2023.11_58_13.AM.webm
Mobile Web - Chrome
mweb_double_paste_-_8_17_2023._12_01_00_pm.Original.mp4
Mobile Web - Safari
mWeb.Safari.-.8_17_2023.12_26_21.PM.webm
Desktop
iOS
Android