Repository navigation
[HOLD for PR #42892] trim leading and trailing CR LF during paste into composer - #41265
kpadmanabhan wants to merge 7 commits into
Conversation
|
@jayeshmangwani 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] |
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
Hey @kpadmanabhan , Are you able to run the app on all the platforms? Please fill in all the missing checkboxes from the description that can be finished. And please write the unit test for the useHtmlPaste function. |
|
@jayeshmangwani Thanks for following up and sorry for delay in update here. Even though my change is not OS dependent, I understand that I must test it in all env. Btw, whom can I contact for a Mapbox key? |
|
@kpadmanabhan For the key, I have responded in Slack. If you need any help in setting it up, please hit me up. |
|
Hey @kpadmanabhan, is there any update for us? If you are stuck anywhere, you can comment here or on #expensify-open-source. |
|
@kpadmanabhan bump for the update |
|
Seems like this PR is abandoned |
|
Yes @roryabraham , it looks like the contributor is not active |
|
@roryabraham / @jayeshmangwani : apologies for this. I was quite unwell for past couple of weeks. Sorry for no updates. Resuming work today. Fixes should be addressed by today e.o.d. |
|
Testing on iOS (native and web), macOS (chrome web and desktop app) complete. Results attached to PR. Where can I find detailed logging on whats going wrong with the launch of simulator and starting the app? |
|
I find I often have to delete the emulator and create new ones. Or if you have access to a physical Android device I find that to be much more reliable than emulators. |
|
Now getting below error. |
|
@kpadmanabhan I haven't gotten the above issue, but I got an issue on Android that was solved by |
|
@kpadmanabhan Any luck with running on Android ? If you still facing the issue, then I would suggest leave the Android And Android mWeb testing for me, I will take care of it, You can focus on remaining part of the PR, And then you can fix your Android environment later. |
|
@jayeshmangwani : thanks for your quick support to investigate the setup. I have fixed the setup and am able to run in Android as well. Updated results of all in the template. Kindly review. |
|
@kpadmanabhan You need to sign the all commits, and lint code is failing here FIX that, remove the commit from a user name |
c3c26ea to
88e08a0
Compare
|
@kpadmanabhan Can we please add the unit test for useHtmlPaste ? |
|
Locally, lint is failing for me for some reason. Does lint pass for you? |
yes. lint is passing for me locally. did you pull latest? |
spacing fix Co-authored-by: Jayesh Mangwani <35371050+jayeshmangwani@users.noreply.github.com>
Yes, it tested in the latest pull; maybe it's only for me |
|
@jayeshmangwani : added unit tests in StringUtils as this is a regex check. Pls re-review. |
|
Thanks @kpadmanabhan for adding the unit tests; I was testing this PR and one scenario I missed before, and I want to confirm before we merge the PR. Right now, in the main branch in composer, when we copy the multi newline and paste it between the large message, then new lines don't lose the originally copied newlines; and now, after adding the above changes in this PR, when we copy the muli new lines text between the large message then leading and trailing newlines got removed, below I am adding two videos for the visual effect of the changes between main and this PR Main branchnew-lines-main.movExisting PRnew-lines-after-this-PR.mov |
@jayeshmangwani : thanks for this test.
|
There must be something wrong with the existing code event that should be fired |
the reason being that start and end of selection range from the node are detected as 0 due to the presence of trailing newline and hence propagation of event does not happen to composer |
|
The last bug reported by @jayeshmangwani does look like a regression that we have to address before we can merge this PR.
Can we pass the text already present in the composer as a param to the hook? |
|
@kpadmanabhan @roryabraham Let's close/HOLD this PR. This proposal was selected for another issue, and that proposal will solve our original issue. |
@jayeshmangwani / @roryabraham : i have fixed the issue you reported by checking for existing hooks |
Technically, we should go with existing PR as this is already in motion, but our changes of replacing the new lines (CR LF) are causing a regression, and the solution on the other issue looks good and does not cause any known regression, so I do think we should go with the other proposal |
|
@jayeshmangwani / @roryabraham : kindly suggest whether I should continue with this PR or put it on hold until other PR is deployed. In which case what will happen to the effort and time spent on this PR? |
@jayeshmangwani / @roryabraham : how should I continue with this? kindly recommend. |
@kpadmanabhan HOLD this PR please, our original proposed changes will cause the regression and other PR is close to getting merged
I will raise this internally and will post here once I get the update cc: @roryabraham |
|
@kpadmanabhan It looks like the issue has been fixed now. I am no longer able to reproduce it. Can you please verify? |
|
|
@kpadmanabhan Please close this PR |
This issue is not reproducible in main HEAD anymore. Closing this PR. |
@jayeshmangwani / @roryabraham
Details
Trimming leading and trailing CR LF characters from pasted content of composer text, so that it does not hamper the flow of
pasteevent triggering and handling in Newdot composer.Fixed Issues
$ #39971
PROPOSAL: #39971 (comment)
Tests
Text(s) tested:
Copy paste an image
Verify that no errors appear in the JS console
Offline tests
Same as above.
QA Steps
Same as above.
Any pasted text (copy to clipboard and paste it into the composer) containing leading or trailing newline Carriage Return (CR) and Line Feed (LF) characters must enable "Send" button of composer.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.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
Android: Native
Screen.Recording.2024-05-23.at.19.59.38.mov
Android: mWeb Chrome
Screen.Recording.2024-05-23.at.20.33.45.mov
iOS: Native
Screen.Recording.2024-05-20.at.19.43.17_1080p.mov
iOS: mWeb Safari
Screen.Recording.2024-05-21.at.21.32.16_720p_updated.mov
MacOS: Chrome / Safari
Screen.Recording.2024-05-20.at.01.10.58_480p.mov
MacOS: Desktop
Screen.Recording.2024-05-20.at.00.55.19_1080p.mov