Skip to content
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

Copy to clipboard functionality #2088

Merged
merged 5 commits into from
Apr 1, 2021

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Mar 25, 2021

Please review @roryabraham

Details

Fixed Issues

Fixes #1778

Tests / QA Steps:

  1. Temporarily edit this function to always return true so that the ReportActionContextMenu will show (see screenshots on the linked issue).
  2. Now click Copy to Clipboard on text, you should have text copied to the clipboard which you can test by pressing ctrl+v
  3. Open the context menu on any attachment message and then click `Copy to Clipboard, you should have html of the message as a string copied to the clipboard which you can test by pressing ctrl+v .

Note: Attachment Copy and especially the image copy feature has been delegated for another Issue. So it won't be covered in this issue.

Possible caveats

  1. On Mobile, long-pressing triggers native text selection action, thus long press on the empty space around the text. (This issue will be covered in another PR as it not linked to this issue.)

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

video.mp4

Mobile Web

1617096056079.mp4

Desktop

screen.mp4

Android

1617098505020.mp4

@parasharrajat parasharrajat requested a review from a team as a code owner March 25, 2021 19:49
@botify botify requested review from Beamanator and removed request for a team March 25, 2021 19:49
@parasharrajat
Copy link
Member Author

parasharrajat commented Mar 25, 2021

@roryabraham. I have converted ReportActionContextMenuItem to the stateful component to show a checkmark. Please have a look at the code and share your feedback, if I am on the right path. Thanks.

Also, copying image on web put this html on clipboard.

<img src="https://www.expensify.com/chat-attachments/728704791/w_160a23711f3558079025957ed57274b2c4f7307c.png.1024.jpg" data-expensify-source="https://www.expensify.com/chat-attachments/728704791/w_160a23711f3558079025957ed57274b2c4f7307c.png" data-name="image.png">

but when I paste this on the ReportActionCompose. There is an error

There was a problem getting the image you pasted. 
NetworkError when attempting to fetch resource.

Any Idea. why?

@roryabraham
Copy link
Contributor

Haven't taken a look at the code yet @parasharrajat (I'm about to), but just FYI we are launching a QA process, so can you please add detailed QA Steps to the PR description too? These should be steps that can be done by our QA engineers on a staging/production environment, so not using any temporary code changes or anything. Thanks! Thats going to be included in the PR template and a standard thing going forward. If it's the same testing steps as in the dev environment, just change that header to:

Tests / QA Steps

@parasharrajat
Copy link
Member Author

parasharrajat commented Mar 26, 2021

Questions:

  1. Do you want the item to do noting after we have shown the checkmark or it should do what it does but the UI will remain in the checked state until the component is remount?
  2. What should we do when the message underneath is an image?

@roryabraham
Copy link
Contributor

Do you want the item to do noting after we have shown the checkmark or it should do what it does but the UI will remain in the checked state until the component is remount

Hmmm I think just doing nothing for now would be fine.

What should we do when the message underneath is an image

Just copy the html to the clipboard I guess.

@parasharrajat
Copy link
Member Author

Just copy the HTML to the clipboard I guess.

Ok you mean just take the HTML variant of the message and Set it as a string on Clipboard.

I found one issue on the Mobile Platform.

  1. When we long-press the text message, the native Text Selection UI opens up. We have to long-press on an empty place to show the Context menu.
  2. When we long press over the image message nothing happens, a single click opens the image Modal.

@roryabraham
Copy link
Contributor

When we long-press the text message, the native Text Selection UI opens up. We have to long-press on an empty place to show the Context menu.

This is a bit tricky to solve because AFAIK the only way to prevent that is giving the <Text> component selectable=false. I think that's fine for iOS and Android, but on mobile web I don't know how to fix it because we want to make sure text is selectable for regular copy&paste on web and desktop.

@parasharrajat
Copy link
Member Author

parasharrajat commented Mar 30, 2021

When we long-press the text message, the native Text Selection UI opens up. We have to long-press on an empty place to show the Context menu.

@roryabraham This issue is related to Context Menu so I think it's better to cover that in another PR. What do you say?

This is a bit tricky to solve because AFAIK the only way to prevent that is giving the component selectable=false. I think that's fine for iOS and Android, but on mobile web, I don't know how to fix it because we want to make sure text is selectable for regular copy&paste on web and desktop.

So in that case, Do you think selectable={!this.props.isSmallScreenWidth} will work?

@parasharrajat parasharrajat changed the title [WIP] Copy to clipboard functionality Copy to clipboard functionality Mar 30, 2021
@parasharrajat
Copy link
Member Author

Ready for review. Thanks.

@roryabraham roryabraham self-requested a review March 30, 2021 17:01
roryabraham
roryabraham previously approved these changes Mar 30, 2021
Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, looks good to me and tested well on iOS. Agreed we can handle the selectable text UX issue in a different issue.

src/libs/Clipboard/index.js Outdated Show resolved Hide resolved
src/pages/home/report/ReportActionContextMenu.js Outdated Show resolved Hide resolved
src/pages/home/report/ReportActionContextMenu.js Outdated Show resolved Hide resolved
src/pages/home/report/ReportActionContextMenuItem.js Outdated Show resolved Hide resolved
src/pages/home/report/ReportActionContextMenuItem.js Outdated Show resolved Hide resolved
src/pages/home/report/ReportActionContextMenuItem.js Outdated Show resolved Hide resolved
src/pages/home/report/ReportActionContextMenu.js Outdated Show resolved Hide resolved
@parasharrajat
Copy link
Member Author

@marcaaron Updated. All yours.

src/libs/reportUtils.js Show resolved Hide resolved
src/libs/reportUtils.js Outdated Show resolved Hide resolved
@parasharrajat
Copy link
Member Author

Updated.

Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great thanks for the changes and you patience!

@marcaaron marcaaron merged commit 366839d into Expensify:master Apr 1, 2021
@OSBotify
Copy link
Contributor

OSBotify commented Apr 1, 2021

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@SameeraMadushan
Copy link
Contributor

Just now noticed when doing pod install, clipboard package changes were not committed in Podfile.lock on this PR

@roryabraham
Copy link
Contributor

Good catch! I feel like the Podfile.lock has been out-of-sync for a while now - any new PR should be able to fix it by running pod install and committing the Podfile.lock, so feel free @SameeraMadushan!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the ability to copy messages in Expensify.cash
5 participants