-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[Pay meow][$250] Web - Composer - Cursor positioned at the start if pasting text which contains a new line #42216
Comments
Triggered auto assignment to @Christinadobrzyn ( |
We think that this bug might be related to #wave-collect - Release 1 |
@Christinadobrzyn FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors. |
Very similar to #42112 - asking if the fix will be the same |
ProposalPlease re-state the problem that we are trying to solve in this issue.Web - Composer - Cursor positioned at the start if pasting text which contains a new line What is the root cause of that problem?We are setting cursor position in below part when pasting. App/src/hooks/useHtmlPaste/index.ts Lines 19 to 20 in fb14d5a
When pasted text contains empty line, the value of What changes do you think we should make in order to solve the problem?We have to set range with What alternative solutions did you explore? (Optional) |
📣 @moonstar-95515! 📣
|
Doesn't seem to be related to #42112 as that is resolved. I can still reproduce this - adding external |
Job added to Upwork: https://www.upwork.com/jobs/~01cc45b59ee9c4ae02 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.Web - Composer - Cursor positioned at the start if pasting text which contains a new line What is the root cause of that problem?In the App/src/hooks/useHtmlPaste/index.ts Line 10 in e6c9f06
What changes do you think we should make in order to solve the problem?Creating a new text variable with stripped newline characters at the end or modifying reusing the same text would work fine.
What alternative solutions did you explore? (Optional)None |
ProposalPlease re-state the problem that we are trying to solve in this issue.when pasting text containing a new empty line into the chat compose box, the cursor incorrectly positions itself at the start of the text instead of the end and a console error occurs. What is the root cause of that problem?The root cause of the problem is that the cursor positioning logic in the insertAtCaret function incorrectly handles the insertion of text containing new lines. Specifically, the cursor is not being moved to the correct position (the end of the newly inserted text) when the text includes empty new lines, which causes the cursor to appear at the start of the text. Additionally, there is no proper error handling to catch and log any potential issues during the paste operation, leading to console errors. What changes do you think we should make in order to solve the problem?a)
modified code:
b)
modified code:
c)
modified code:
Summary of Changes1.Cursor Positioning Fix: Updated the insertAtCaret function to use range.setStartAfter(node) and range.setEndAfter(node) to correctly position the cursor at the end of the newly inserted text, especially when new lines are involved. 2.Error Handling: Added console.error logging within the paste function to handle and log any exceptions that occur during the paste operation, making it easier to debug issues. What alternative solutions did you explore? (Optional)None Contributor details |
📣 @RickDeb2004! 📣
|
hi @eVoloshchak can you take a peek at these proposals when you have a moment? TY! |
I think we should proceed with @rakhaxor's proposal, it's clean and straightforward. It does cut off all of the new lines at the end of the pasted text, but that's what the backend does when you send the message, so this can be considered a small improvement 🎀👀🎀 C+ reviewed! |
Triggered auto assignment to @AndrewGable, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
📣 @rakhaxor You have been assigned to this job! |
My proposal works when it has 3 or more new lines at the end, as the text will be |
@eVoloshchak cc: @Christinadobrzyn @AndrewGable
I can't sure if I understood correctly, but I think only new lines at the end of the text will be cut on backend side. test.mp4 |
@AndrewGable, @eVoloshchak, @rakhaxor, @Christinadobrzyn, @moonstar-95515 Whoops! This issue is 2 days overdue. Let's get this updated quick! |
Hi @moonstar-95515 sorry for the delay here - I just created a new offer in this Upwork job post - https://www.upwork.com/nx/wm/offer/103512886 Can you see if you can accept that for payment? Thank you! |
@Christinadobrzyn |
Thanks @moonstar-95515! I'm a little lost on the payments due for this. I think this is the summary, can we get a review. Payouts due:
|
@Christinadobrzyn |
@rakhaxor had an accepted proposal and was initially assigned/hired so $125 seems fair, even with @moonstar-95515 working on the PR/issue later. |
@mallenexpensify Of course, I know that I have no face to say anything because my solution has not been reflected in production. |
Hello @Christinadobrzyn, Thank you for the offer. Here’s my upwork: https://www.upwork.com/freelancers/~01f94219e6101472c1 |
@mallenexpensify , @Christinadobrzyn Here's summary:
I think I put much more effort in this issue and the same compensation is not fair. |
@moonstar-95515 What’s your problem with me man? I have done all the tests and made screen recordings. You were assigned later on, I did all the work there and was asking to submit PR. I didn’t even comment even after doing all the work and not being able to submit it until now when I was asked to submit my profile link. Anyways, I just need you to not talk about me anymore. Thank you |
@rakhaxor |
The decision to compensate @rakhaxor is an edge case, based on the fact they were assigned to the issue and hired. It was the responsibility of Expensify to hire/assign. Since this action is considered a source-of-truth and gives the green light for the contributor to invest more time into the issue (assuming compensation), we feel it's fair to compensate. Since their proposal was initially accepted, we decided on 50%. @moonstar-95515 , your compensation on this issue, and the value associated with it, is completely unrelated to the compensation for @rakhaxor. Please keep in mind our documented code of conduct when communicating with others, both internal and with fellow contributors.
Thanks |
@AndrewGable, @eVoloshchak, @rakhaxor, @Christinadobrzyn, @moonstar-95515 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
Quick follow up - based on a review (as Matt outlined) we are going to continue with this payment summary. @eVoloshchak can you accept this Upwork offer - https://www.upwork.com/nx/wm/offer/103544327 TY! |
Hello @Christinadobrzyn! I have accepted the offer on Upwork. Thank you |
@Christinadobrzyn, declined the Upwork offer, requested the payment on NewDot |
Awesome! Thanks! The payment summary is here. Feel free to submit your NewDot payment @eVoloshchak. Also, I don't see that it was asked but do we need a regression test for this? |
$250 approved for @eVoloshchak |
@eVoloshchak can you let us know about a regression test? Then I think this is good to close! |
Yes, a regression test would be useful and easy to perform Regression Test Proposal
Do we agree 👍 or 👎 |
Regression test is created - https://github.com/Expensify/Expensify/issues/422290 Closing! |
@AndrewGable @Christinadobrzyn Be sure to fill out the Contact List! |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Version Number: 1.4.74-0
Reproducible in staging?: y
Reproducible in production?: y
Issue reported by: Applause - Internal Team
Action Performed:
Expected Result:
The cursor gets positioned at the end of the pasted text, no console error
Actual Result:
The cursor gets positioned at the start of the pasted text if the pasted text contains a new empty line, console error is showing
Workaround:
n/a
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
Bug6481716_1715782332852.bandicam_2024-05-15_17-07-29-894.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: