[Hold for Payment - Sep 7th] Web - Attachments Preview - Zooming in the edges of an image will lead to white spaces

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Open any chat
2. Send an image as attachment
3. Click on it in order to have larger preview
4. Click on any edges in order to zoom it

Expected Result:

Picture is zoomed and picture fills whole preview dialogue

Actual Result:

Picture is zoomed, but there is a lot of white spaces

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platform:

Where is this issue occurring?

Web ✔️
iOS
Android
Desktop App
Mobile Web

Version Number:
1.0.75-5
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Bug5142406_video.mp4

Expensify/Expensify Issue URL: https://github.com/Expensify/Expensify/issues/170015

Upwork job posting: Posted to Upwork: https://www.upwork.com/jobs/~014d8cadbc82d362b0

View all open jobs on Upwork

[MelvinBot]

Triggered auto assignment to @francoisl (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[francoisl]

Looks like this can be external

[MelvinBot]

Triggered auto assignment to @NicMendonca (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[rdjuric]

What's the expected behavior here? Looks like the app kinda centers the image on the point where we're zooming in.
https://github.com/Expensify/Expensify.cash/blob/f11d808f6358ba57ae02a1d392229dbd38491afd/src/components/ImageView/index.js#L95-L104
I feel like this is the correct behavior, but it also means that if we zoom on the edge there will be blank spots.

[NicMendonca]

@rdjuric that question might be better suited for #expensify-open-source.

[MelvinBot]

Triggered auto assignment to @roryabraham (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[roryabraham]

What's the expected behavior here

My thinking here is that we should center the image on the point we're zooming in on, but if we're within a specific distance of any edge, then we'd shift the zoomed-in section accordingly. Something like this:

So if the point where we've clicked (the center of the zoom) is in any of the "gutters", then we'll shift the image horizontally and/or vertically as needed so that the zoomed-in size will fit within the bounds of the image.

[isagoico]

Issue reproducible during KI retests

[NicMendonca]

Hi @rdjuric! Based off @roryabraham's comment, are you keen to submit a proposal for this?

[rdjuric]

@NicMendonca Still researching this one 😅

Thanks for the picture, @roryabraham. So I think the behavior we want is what we have in the video below?

desktop.mov

[roryabraham]

It looks to me like it might not be working perfectly in the y-axis, but yeah that's prettymuch what I had in mind.

[roryabraham]

I have to reassign this to someone else while I'm OOO

[MelvinBot]

Triggered auto assignment to @tgolen (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[isagoico]

Issue reproducible during KI retests

[MTN718]

Yeah, then it makes sense.

Do you agree it's solution for issue?

[parasharrajat]

I don't see anything proposed yet. It seems more of an analysis. Could you please post a formal proposal? you can look at contribution guidelines to learn about creating a proposal.

---

I am not an assigned engineer for this Task. So It does not matter if I think or not.

[MTN718]

Proposal (Already submitted on upwork)

My name is Shane Watson from South Africa.
I suggest a way to fix issue following

1. Get Image Size when attachment view component mounted
2. Passing Image width/height parameter for zoom style
3. Calculate scale base on max width
4. Create new zoom width/height instead of 250% of view
5. Change Image resize mode property to "contains"

I already tested and you can see result above

[roryabraham]

Thanks for your analysis @MTN718. I think what you've done so far make sense, but it's very hard to tell from your screenshots because the image you've chosen has a white background. Can you please post a screenshot with a dark background so it's easy to see that the whitespace issue is eliminated? Thanks!

[MTN718]

Added Videos for different size of images

Screen.Recording.2021-08-20.at.2.14.41.PM.mov

Screen.Recording.2021-08-20.at.2.17.39.PM.mov

Screen.Recording.2021-08-20.at.2.18.22.PM.mov

[roryabraham]

@MTN718 Looks great! Thanks for submitting those extra videos. @NicMendonca please hire @MTN718 for this issue on Upwork, and @MTN718 feel free to submit a pull request with the code changes as soon as you've been hired on Upwork.

[MTN718]

My Upwork account name is Shane Watson from South Africa

[NicMendonca]

@MTN718 hired!

[MTN718]

Thanks. Let me create PR

[MTN718]

Created PR. Waiting Feedback

[isagoico]

Issue reproducible during KI retests

[MTN718]

You mean issue again from PR?
@roryabraham PR already merged in test?

[roryabraham]

PR is not merged yet, but I just re-reviewed it.

[MTN718]

@roryabraham You will let me know when PR merged? or already merged?

[isagoico]

Issue reproducible during KI retests

[MTN718]

@roryabraham my PR deployed in current KI retests?

[roryabraham]

Not yet

[botify]

🚀 Deployed to staging by @robertjchen in version: 1.0.88-3 🚀

platform	result
🤖 android 🤖	cancelled 🔪
🖥 desktop 🖥	cancelled 🔪
🍎 iOS 🍎	cancelled 🔪
🕸 web 🕸	success ✅

[botify]

🚀 Deployed to production by @roryabraham in version: 1.0.90-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

[MTN718]

@NicMendonca Issue resolved?

[MTN718]

@NicMendonca @roryabraham any update on upwork?

[NicMendonca]

paid!