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

[HOLD for payment 2024-08-23] [$2000] Add support for copy/pasting images on iOS #41239

Closed
roryabraham opened this issue Apr 29, 2024 · 127 comments
Closed
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item. Reviewing Has a PR in review Weekly KSv2

Comments

@roryabraham
Copy link
Contributor

roryabraham commented Apr 29, 2024

slack proposal: https://expensify.slack.com/archives/C01GTK53T8Q/p1714239125412939

Feature Request: Add support for pasting images on iOS.

Problem

One of the most common flows for sharing images on iOS is to copy the image somewhere and paste it somewhere else, and NewDot does not support that flow. For example, I wanted to share an image I found on Google in New Expensify. The first thing I did was copy the image to my clipboard, but when I long-pressed on the composer to try and paste that image, the only option available was AutoFill.
This left me with two more less desirable options:

  • copy the URL to the image, then write that URL in a markdown comment with inline image syntax (this was the next thing I tried, and it actually didn't work for the URL I had)
  • save the image to my photos, then upload it to New Expensify. This is less ideal because it pollutes my photos gallery with something I don't want to keep there.

This was a very basic flow that did not work very well at all, and it was a frustrating experience. It's table-stakes for any chat app.

Solution

Implement support for pasting images on iOS.

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0195bbb40518505764
  • Upwork Job ID: 1790820991593271296
  • Last Price Increase: 2024-07-16
  • Automatic offers:
    • ahmedGaber93 | Reviewer | 103152998
Issue OwnerCurrent Issue Owner: @
Issue OwnerCurrent Issue Owner: @sonialiap
@roryabraham roryabraham added External Added to denote the issue can be worked on by a contributor Daily KSv2 labels Apr 29, 2024
@roryabraham roryabraham self-assigned this Apr 29, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 29, 2024
Copy link

melvin-bot bot commented Apr 29, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @ahmedGaber93 (External)

@roryabraham roryabraham added the NewFeature Something to build that is a new item. label Apr 29, 2024
Copy link

melvin-bot bot commented Apr 29, 2024

Triggered auto assignment to @sonialiap (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

@melvin-bot melvin-bot bot added the Weekly KSv2 label Apr 29, 2024
Copy link

melvin-bot bot commented Apr 29, 2024

⚠️ It looks like this issue is labelled as a New Feature but not tied to any GitHub Project. Keep in mind that all new features should be tied to GitHub Projects in order to properly track external CAP software time ⚠️

Copy link

melvin-bot bot commented Apr 29, 2024

Triggered auto assignment to Design team member for new feature review - @dubielzyk-expensify (NewFeature)

@roryabraham
Copy link
Contributor Author

I don't think we need any design review here

@ShridharGoel
Copy link
Contributor

ShridharGoel commented Apr 30, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

Add images copy/paste in iOS.

What is the root cause of that problem?

New feature.

What changes do you think we should make in order to solve the problem?

Add a new option "Paste image" in the + menu of chats, which will show only when there's an image copied in the clipboard.

Clicking on this option will open the attachment modal with the image.

const [imageToBePasted, setImageToBePasted] = useState('')

useEffect(() => {
    Clipboard.getString().then((data) => {
        if (/\.(jpg|png|jpeg|svg)(\?.*)?$/.test(data)) {
            setImageToBePasted(data);
        }
    });
})

Like we have displayFileInModal, we'll add displayFileInModalUsingUrl which will have url of the copied image as input.

New option in attachment picker:

if (imageToBePasted) {
    menuItems.push({
        icon: Expensicons.Paperclip,
        text: 'Paste image',
        onSelected: () => {
            if (Browser.isSafari()) {
                return;
            }
            displayFileInModalUsingUrl(imageToBePasted)
    }})
}
Screen.Recording.2024-04-30.at.12.38.36.PM.mov

Note that the above code is not extensive, I had to do more changes locally to make it work.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Apr 30, 2024
@dubielzyk-expensify
Copy link
Contributor

Love this ticket. As for the solution, I kinda find it weird to trigger the "paste" permission on (+) click. Does this only work when you have copied a picture? What happens if you copy text?

In other apps, and even for Expensify Web and MacOS, you can paste a picture into the actual compose bar text input which to me feels more natural than to click the (+) button. Thoughts? cc @Expensify/design

@ShridharGoel
Copy link
Contributor

Does this only work when you have copied a picture? What happens if you copy text?

This option would only show when we have an image URL in the clipboard.

"paste" permission

That is needed because we want to get the clipboard content.

@ShridharGoel
Copy link
Contributor

you can paste a picture into the actual compose bar text input

That might not be feasible in iOS. Maybe we can modify the above shown flow instead?

@trjExpensify
Copy link
Contributor

🤔 Also, what about Android to maintain cross-platform consistency given @Beamanator's recent discovery here on Rory's OG issue for this:

I did figure out that on an android device, you can go to chrome, search for an image, press & hold, and copy to the clipboard -> so shouldn't we make sure this is possible on our android app too? 👍

@shawnborton
Copy link
Contributor

100% agree, I find it super bizarre to override the default + behavior here. We should definitely not do that.

Why can't you just paste into the compose box as if you were pasting text, and then that triggers the paste dialog?

@ShridharGoel
Copy link
Contributor

When someone adds an image link in the input box, we can open the attachment modal with the image. But what if the user just wants to send the link itself? Should we show a modal with the option to add the image as an attachment or just keep it as a link?

@shawnborton
Copy link
Contributor

Well if a link was on your clipboard, I would think you would just paste the link. But if an image was on your clipboard, then it makes sense to trigger the modal window.

@ShridharGoel
Copy link
Contributor

When I check the clipboard content after copying an image on iOS, it just shows the link.

@ahmedGaber93
Copy link
Contributor

ahmedGaber93 commented Apr 30, 2024

@ShridharGoel Thanks for the proposal.

I think pasting the image from the URL in the clipboard seem a workaround, because when we copied the image, we click "copy image" not "copy image address", so we may need to handle the image as fileObject or base64 string.

Also, your proposal doesn't have how we will detect onPasteFile on the normal case by long click on the input then click paste.

@dubielzyk-expensify
Copy link
Contributor

The paste to compose bar is how most apps on iOS works. It works like this on Messages, Telegram, and Signal. So I suggest we do that. Like Shawn said, let's trigger the modal window like we do on web/mac os.

@melvin-bot melvin-bot bot added the Weekly KSv2 label Aug 24, 2024
@roryabraham
Copy link
Contributor Author

Ready to pay

@s77rt
Copy link
Contributor

s77rt commented Aug 25, 2024

Can we not apply the regressions penalties here due to complexity?

@roryabraham
Copy link
Contributor Author

Yes, that seems reasonable in this case, as long as you fix regressions that do come up. I see you created another PR already, so thank you for that.

Copy link

melvin-bot bot commented Aug 27, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@s77rt
Copy link
Contributor

s77rt commented Aug 27, 2024

False positive ^

@stephanieelliott
Copy link
Contributor

stephanieelliott commented Aug 28, 2024

OK, I will go ahead and pay this now seeing as we're not going to adjust for regressions.

Summarizing payment on this issue:

  • Contributor: @ahmedGaber93 $2000 via Upwork - PAID
  • Contributor: @ishpaul777 paid $500 via Upwork.
  • Contributor+: @s77rt $2000 via NewDot - please request!

Upwork job is here: https://www.upwork.com/jobs/~0195bbb40518505764

@ahmedGaber93
Copy link
Contributor

ahmedGaber93 commented Aug 28, 2024

@stephanieelliott payment summary was changed and summarized by @sonialiap here #41239 (comment) to split 25% of C+ bounty to @ishpaul777 for debugging and reviewing a case on a iOS real device

@JmillsExpensify
Copy link

$2,000 approved for @s77rt based on this first summary.

@ishpaul777
Copy link
Contributor

Gentle bump @stephanieelliott on #41239 (comment)

@s77rt s77rt mentioned this issue Aug 30, 2024
50 tasks
@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Aug 30, 2024
@mallenexpensify mallenexpensify added Daily KSv2 and removed Reviewing Has a PR in review Weekly KSv2 labels Aug 30, 2024
@mallenexpensify
Copy link
Contributor

Paid @ishpaul777 , updated payment summary above.

@ahmedGaber93 , requested $500 refund, per the split bounty, please confirm via a comment here once you've issued it. Thx

@ahmedGaber93
Copy link
Contributor

please confirm via a comment here once you've issued it. 

@mallenexpensify Done.

@mallenexpensify
Copy link
Contributor

Thanks @ahmedGaber93 , confirming refund has been addressed.
@s77rt do we want a regression test from here? If not, why? Thx

@melvin-bot melvin-bot bot added the Overdue label Sep 2, 2024
@s77rt
Copy link
Contributor

s77rt commented Sep 3, 2024

@mallenexpensify This is a new feature and it does require regression test.

Regression Test Proposal

  1. Copy image (from Gallery or Google search results)
  2. Open any report
  3. Paste the image
  4. Verify that an attachment modal is displayed
  5. Upload the photo
  6. Verify the photo is uploaded with success

@sonialiap
Copy link
Contributor

Thanks Steph and Matt for handling payment while I was OOO!

Submitted Regression test to the QA team. I believe this is all done and ready to close

@melvin-bot melvin-bot bot removed the Overdue label Sep 3, 2024
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item. Reviewing Has a PR in review Weekly KSv2
Projects
No open projects
Development

No branches or pull requests