-
Notifications
You must be signed in to change notification settings - Fork 3k
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 for double paste in composer after function component mig #25419
Fix for double paste in composer after function component mig #25419
Conversation
@@ -363,18 +363,18 @@ function Composer({ | |||
} | |||
|
|||
if (textInput.current) { | |||
textInput.current.addEventListener('paste', handlePaste); | |||
document.addEventListener('paste', handlePaste); |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
asked in open source here https://expensify.slack.com/archives/C01GTK53T8Q/p1692292251654389
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI Here is the issue of multiple composers #23502
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a regression from #23359
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.
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 |
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.
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 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
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