6326: fix bug reported at #6518 by railway17 · Pull Request #6535 · Expensify/App

railway17 · 2021-11-30T13:17:30Z

Details

Regress bugs that are displayed when testing with very long or wide images

Tested On

Screenshots

Web

Web Regression Video

Desktop

Desktop Regression Video

deetergp

Code looks good, but I had a couple comments regarding comments.

deetergp · 2021-11-30T19:35:54Z

src/components/ImageView/index.js

            imgRight = imgLeft + (fitRate * width);
        }

+        //In case image loading is delayed than onLayout callback of the root View caused internet speed


This comment isn't exactly clear. Is it still relevant/needed?

Actually, my internet speed is not good sometimes. And I noticed that it has some issue because we would not load image correctly.
If you don't need this, I will remove.

deetergp · 2021-11-30T21:30:25Z

src/styles/styles.js

+
+    // Return different size and offset style based on zoomScale and isZoom
    if (isZoomed) {
+        if (zoomScale > 1) {


Would you please note what's going on in the UI in each of these cases?

I added the comments for each styling logic.
And noticed that my styling was not correct in the previous commit.
So, fixed it too. (when isZoom === false && zoomScale < 1)
Please review and let me know if something is still not clear.
Thank you

That's perfect. Thanks!

deetergp · 2021-12-01T22:31:42Z

Approved, but it looks like you've got a couple merge conflicts to resolve.

railway17 · 2021-12-02T00:47:37Z

railway17 · 2021-12-02T00:51:31Z

Approved, but it looks like you've got a couple merge conflicts to resolve.

Oh.. sorry
I had mistake while checking this page.
Clicked the close pull request button unexpectedely.

railway17 · 2021-12-02T01:01:28Z

Approved, but it looks like you've got a couple merge conflicts to resolve.

Hi, @deetergp
Do you want me to merge this PR to main by resolving conflicts myself?

# Conflicts: # src/components/ImageView/index.js # src/styles/styles.js

railway17 · 2021-12-02T07:21:03Z

Hi, @deetergp
Because there was a problem resolving conflict in GitHub, I did it locally. (I noticed that my style code was moved to StyleUtil.js).
Please review it and let me know if you have any other problem
Thank you

parasharrajat

Could you please follow the template for the PR?

$ Original Issue
$ Regression Issue

Revert these to how they should be.

parasharrajat · 2021-12-02T17:02:09Z

src/components/ImageView/index.js

-        this.setState({
-            imgWidth: width, imgHeight: height, imageLeft: imgLeft, imageTop: imgTop, imageRight: imgRight, imageBottom: imgBottom,
-        });
+        // In case image loading is delayed than onLayout callback of the root View caused internet speed.


I didn't really understand what this comment is about?

For regression, there is new issue that is open.
Because I didn't know which issue number should be written, so I made it.
Which issue number should be written?
Original one?

For this comment, I noticed that image loading is delayed because of internet speed.
In this case, image size is not decided even though the container (modal) is open.
Because of this reason, zoomScale is also not decided.
To avoid this problem, I wrote the code and comment like above.

onLayout callback of the root View caused internet speed.

this is not making sense to me. Could you please update it?

Please mention both issue URLs in the description.

It means
1: You don't need such exception handling? Need to remove that code?
2. Should not write as Original issue, Regression issue?
Instead of it, should write urls directly like below?
https://github.com/Expensify/App/issues/6326, https://github.com/Expensify/App/issues/6518

Hi, @parasharrajat
Could you please let me know what you expect exactly?

I will let others review it. I don't really know this part of code.

If you want, I can remove that code shortly.
But I added that code because I noticed the issue in my side.
Please let me know whenever you have any feedback.

Hi, @parasharrajat , @deetergp
How are you doing?
Any feedback from other reviewers?
Please kindly let me know so that we can proceed.
Thank you

I tried hard to understand what you were trying to communicate with this comment, but it doesn't make sense to me. It sounds like there's some race condition at play here? If you can explain what you mean in a different way, that would be great. Otherwise, please just remove this comment.

parasharrajat · 2021-12-06T09:48:47Z

I am not familiar with this part of code thus I would not be able to properly review it. I am also not assigned to this issue. Please continue without my review.

I would recommend to get more eyes on this PR as there could be more regressions.

railway17 · 2021-12-06T09:56:00Z

I am not familiar with this part of code thus I would not be able to properly review it. I am also not assigned to this issue. Please continue without my review.

I would recommend to get more eyes on this PR as there could be more regressions.

@parasharrajat
Thank you for your quick reply.

Approved, but it looks like you've got a couple merge conflicts to resolve.

@deetergp
Could you approve this PR again?
You have already approved this PR but it needs another approval while I merge with main in my local.
I think it is caused by my bad activity.
Please check my merging (I merged correctly but you can double-check) and approve so that we can proceed.
Thank you

railway17 · 2021-12-06T17:02:28Z

@roryabraham
What means your assignment?
Did something go wrong?

roryabraham · 2021-12-06T17:07:54Z

Hi @railway17. I'm going to review this – this is time-sensitive so let's work together to get this merged ASAP.

railway17 · 2021-12-06T17:09:31Z

Hi @railway17. I'm going to review this – this is time-sensitive so let's work together to get this merged ASAP.

Ok, here is 1:00am now, but I will try to work with you together.
Let's start now.

roryabraham

Also, what if the window resizes? I think maybe we need to handle this by:

Saving the image height as a class field before calling setImageRegion
Calling setImageRegion again in componentDidUpdate if the window dimensions changed.

roryabraham · 2021-12-06T17:48:37Z

src/components/ImageView/index.js

+        const newZoomScale = Math.min(this.state.containerWidth / width, this.state.containerHeight / height);
+        this.setState(prevState => ({
+            imgWidth: width,
+            zoomScale: prevState.zoomScale === 0 ? newZoomScale : prevState.zoomScale,


Why not just use the newZoomScale in all cases?

Actually, I was going to set zoomScale in the onLayout event of the parent view.
But while testing in my local, I noticed that it is sometimes 0 because image loading is slower than view loading.
So, reset with newZoomScale here.

roryabraham · 2021-12-06T17:51:46Z

src/components/ImageView/index.js

-        this.setState({
-            imgWidth: width, imgHeight: height, imageLeft: imgLeft, imageTop: imgTop, imageRight: imgRight, imageBottom: imgBottom,
-        });
+        // In case image loading is delayed than onLayout callback of the root View caused internet speed.


I tried hard to understand what you were trying to communicate with this comment, but it doesn't make sense to me. It sounds like there's some race condition at play here? If you can explain what you mean in a different way, that would be great. Otherwise, please just remove this comment.

roryabraham · 2021-12-06T17:54:42Z

@railway17 It's okay – what I'm going to do instead is revert your original PR and that way we can take the time we need to re-implement it without causing a regression.

railway17 · 2021-12-06T18:00:21Z

@railway17 It's okay – what I'm going to do instead is revert your original PR and that way we can take the time we need to re-implement it without causing a regression.

I removed the comment from that code and pushed it.
It is under checking now.

roryabraham · 2021-12-06T18:03:55Z

I removed the comment from that code and pushed it.
It is under checking now.

Okay, I had a couple more comments in my last review

railway17 · 2021-12-06T18:08:19Z

setImageRegion

2. setImageRegion
Actually, this issue requirement was fixing cursor icon issue while zoomin or zoomout for small thumbnails.
So, I didn't consider about window resize.

roryabraham

Okay, I'm going to approve and CP this to unblock the deploy, but will follow up with a small PR of my own to address my comments.

roryabraham · 2021-12-06T18:10:23Z

CP'ing to unblock deploys.

botify · 2021-12-06T18:10:34Z

@roryabraham looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

github-actions · 2021-12-06T18:11:28Z

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

(cherry picked from commit d445fda)

roryabraham · 2021-12-06T18:15:09Z

Not an emergency, just CP'ing to speed up deploys. Only change since last successful E2E test was the removal of a comment.

OSBotify · 2021-12-07T00:20:23Z

🚀 Deployed to staging by @roryabraham in version: 1.1.17-8 🚀

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

OSBotify · 2021-12-08T20:12:15Z

🚀 Deployed to production by @roryabraham in version: 1.1.18-3 🚀

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

parasharrajat · 2021-12-15T16:45:10Z

We have found another regression from this PR. @railway17

cc: @roryabraham

6326: fix bug reported at Expensify#6518

73afc16

railway17 requested a review from a team as a code owner November 30, 2021 13:17

MelvinBot requested review from deetergp and removed request for a team November 30, 2021 13:17

deetergp requested changes Nov 30, 2021

View reviewed changes

add comments for each styling logic

c94fbf0

deetergp previously approved these changes Dec 1, 2021

View reviewed changes

railway17 closed this Dec 2, 2021

railway17 deleted the han-fix-magnifying-glass branch December 2, 2021 00:47

revert to unexpected closing pull request

2167b00

railway17 reopened this Dec 2, 2021

railway17 dismissed deetergp’s stale review via 2167b00 December 2, 2021 00:56

railway17 force-pushed the han-fix-magnifying-glass branch from 87eac47 to 2167b00 Compare December 2, 2021 01:59

Merge branch 'main' into han-fix-magnifying-glass

11fce35

# Conflicts: # src/components/ImageView/index.js # src/styles/styles.js

parasharrajat suggested changes Dec 2, 2021

View reviewed changes

isagoico mentioned this pull request Dec 2, 2021

Deploy Checklist: New Expensify 2021-11-29 #6505

Closed

69 tasks

roryabraham assigned railway17 Dec 6, 2021

roryabraham self-requested a review December 6, 2021 17:07

roryabraham requested changes Dec 6, 2021

View reviewed changes

roryabraham mentioned this pull request Dec 6, 2021

Fix: Blurred images in attachment view on mobile devices #6442

Merged

5 tasks

remove comment

c98a78d

roryabraham approved these changes Dec 6, 2021

View reviewed changes

roryabraham added the CP Staging label Dec 6, 2021

roryabraham merged commit d445fda into Expensify:main Dec 6, 2021

botify added the Emergency label Dec 6, 2021

github-actions bot pushed a commit that referenced this pull request Dec 6, 2021

Merge pull request #6535 from railway17/han-fix-magnifying-glass

caf6dc5

(cherry picked from commit d445fda)

OSBotify mentioned this pull request Dec 6, 2021

🍒 Cherry pick PR #6535 to staging 🍒 #6596

Merged

roryabraham removed the Emergency label Dec 6, 2021

roryabraham mentioned this pull request Dec 6, 2021

Make ImageView zoom scale dynamic with window size #6599

Closed

5 tasks

botify mentioned this pull request Dec 6, 2021

Chat - Magnifying glass displayed anywhere on the modal page for small images #6326

Closed

OSBotify mentioned this pull request Dec 6, 2021

Deploy Checklist: New Expensify 2021-12-06 #6605

Closed

95 tasks

This was referenced Dec 6, 2021

Web - Attachments - Unable to get to the top of images with long height and not able to zoom #6518

Closed

Do not include CP merge commits in the list of PR merge commits #6165

Merged

iwiznia mentioned this pull request Dec 14, 2021

[Reporting bonus paid] Attachment view shows image as left aligned. It should be horizontally centered - Reported by: @Prashant Mangukiya #6714

Closed

OSBotify mentioned this pull request Aug 16, 2022

Deploy Checklist: New Expensify 2022-08-04 #10253

Closed

Conversation

railway17 commented Nov 30, 2021

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Desktop

Uh oh!

deetergp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deetergp commented Dec 1, 2021

Uh oh!

railway17 commented Dec 2, 2021

Uh oh!

railway17 commented Dec 2, 2021

Uh oh!

railway17 commented Dec 2, 2021

Uh oh!

railway17 commented Dec 2, 2021

Uh oh!

parasharrajat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parasharrajat Dec 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

railway17 Dec 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parasharrajat commented Dec 6, 2021

Uh oh!

railway17 commented Dec 6, 2021

Uh oh!

railway17 commented Dec 6, 2021

Uh oh!

roryabraham commented Dec 6, 2021

Uh oh!

railway17 commented Dec 6, 2021

Uh oh!

roryabraham left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parasharrajat Dec 2, 2021 •

edited

Loading

railway17 Dec 3, 2021 •

edited

Loading

parasharrajat commented Dec 15, 2021 •

edited

Loading