[$500] Chat - Loading wheel for large image is not completely visible

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.4.55-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4441489
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

Pre-requisite: the user must be logged in

1. Go to any chat
2. Upload the attached large image
3. When the image finally uploads, verify that when it loads, the loading wheel is not completely visible

Expected Result:

The loading wheel should be fully visible

Actual Result:

The loading wheel is almost completely hidden

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6420675_1710950197284.Oogr9269_1_.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~010fa06a3d83b43db6
• Upwork Job ID: 1770603734538096640
• Last Price Increase: 2024-03-28
• Automatic offers:
• dukenv0307 | Reviewer | 0
• tienifr | Contributor | 0

[melvin-bot]

Triggered auto assignment to @johncschuster (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[lanitochka17]

@johncschuster FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[lanitochka17]

We think that this bug might be related to #vip-vsp

[abzokhattab]

sounds like an edge case, no?

[johncschuster]

Yeah, this feels like a pretty extreme case to me. I think we can probably close this. (Discussing here)

[johncschuster]

Welp! After discussing, we feel it's worth fixing. I've added it to #vip-vsb

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~010fa06a3d83b43db6

[melvin-bot]

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

[Krishna2323]

Proposal

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

Chat - Loading wheel for large image is not completely visible

What is the root cause of that problem?

We don't have min width on the View containing ImageWithSizeCalculation.

App/src/components/ThumbnailImage.tsx

Lines 99 to 100 in 1a8b66a

	<View style={[...sizeStyles, styles.alignItemsCenter, styles.justifyContentCenter]}>
	<ImageWithSizeCalculation

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

Add minWidth & minHeight so there is enough width for the loading indicator. The values can be decided by the design team.

Also need to add here:

App/src/components/ThumbnailImage.tsx

Lines 84 to 85 in 1a8b66a

	<View style={[style, styles.overflowHidden, styles.hoveredComponentBG]}>
	<View style={[...sizeStyles, styles.alignItemsCenter, styles.justifyContentCenter]}>

Note

I have tested the my solution and it does work perfectly without any flickering and on all device sizes, we just need to update the minWidth & minHeight size on small width devices.

Result

Slack:

slack_demo.mp4

Fix:

fix_demo_long_image.mp4

Alternatively

We can pass ...sizeStyles to ImageWithSizeCalculation using style prop and inside ImageWithSizeCalculation we will set the minWidth until isLoading && !isImageCached is true.

[Krishna2323]

Proposal updated

[tienifr]

Proposal

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

The loading wheel is almost completely hidden

What is the root cause of that problem?

In here, we have the logic to set the image size, if the height is larger than width like in this case, the height will be fixed, and the width will be calculated based on the aspectRatio of the image.

The problem here is this image is much larger in height than in width, causing the width to become very small it's almost invisible. The loading indicator will also not be visible.

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

In the image size calculation logic, we need to cap the minimum width and minimum height of the image as well, by capping the aspectRatio, so the if the image width according to aspectRatio is less than minimum, it will take the minimum size due to the capped aspectRatio

To do this, we can update here

const aspectRatio = (height && Math.max(width / height, 0.3)) || 1;

(Do the same for height as well, it can be as easy as lodashClamp(width / height, 0.3, 1 / 0.3))

This will make sure that the image width displayed will be at least 30% of image height, and vice versa (we can adjust the percentage as we see fit). Do note that the fixed dimension of the image is different for small screen and large screen here, so a fixed minWidth/minHeight will not work since it produces different & inconsistent "capped aspect ratio" on different screen sizes. Meanwhile, the aspectRatio will cap by percentage, ensuring that the capped ratio will be the same regardless of screens.

This will make sure the image looks well not only in loading but also after loaded. Otherwise it will cause a flicker in size after the image is loaded, and also without this, the image after loaded is barely visible.

This is the same UX that Slack has, try sending the long image to Slack and you'll see the same (the minimum height is capped both before and after the image is loaded)

What alternative solutions did you explore? (Optional)

The issue mentioned here is a different bug and out of the issue's scope. We should fix it separately (probably by still rendering the image container that has the minimum width, height, if the image's actual width & height are small than that containers's we should show the image at its intrinsic width, height, but contained in the center of the outer container). This is an edge case also, I don't think regular users would send such small image, so I'm not sure it's worth fixing.

[Krishna2323]

Proposal Updated

Added slack UX and fix result.

[tienifr]

Proposal updated to add example code change

[Krishna2323]

Proposal Updated.

Added a note and we also need to set the maxHeight.

[dukenv0307]

Thanks for all your proposals

@Krishna2323 Your solution can fix the issue but I don't like using fixed min-width or min-height it can cause the inconsistency aspect-ratio on large device and small device, let's see the image below when I set min-width: 200px, they look so different

@jsdev2547 Your solution cause the flicker between loading state and loaded state

@tienifr's solution seems fine to me. We should adjust the minimum to lower than 0.3, maybe 0.15 is good.

🎀👀🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @youssef-lr, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[Krishna2323]

@dukenv0307, I haven't suggested to use 200px :), just like slack we will use something like 52 or 72. Just to make enough space for loader. I tested my solution on small width devices and it works fine.

Also I think the selected proposal won't work with wide images with short height.

[dukenv0307]

I haven't suggested to use 200px :)

I mean your solution can cause the inconsistency, I want to use 200px to make it clearer. The reason behind that is here:

App/src/hooks/useThumbnailDimensions.ts

Line 17 in 0aa0ffa

const fixedDimension = isSmallScreenWidth ? CONST.THUMBNAIL_IMAGE.SMALL_SCREEN.SIZE : CONST.THUMBNAIL_IMAGE.WIDE_SCREEN.SIZE;

fixedDimension depends on screen size

Also I think the selected proposal won't work with wide images with short height.

Can you give me an example?

[tienifr]

Also I think the selected proposal won't work with wide images with short height.

@Krishna2323 It will work just fine. Perhaps you didn't see the below part of my proposal.

(Do the same for height as well, it can be as easy as lodashClamp(width / height, 0.3, 1 / 0.3))

[Krishna2323]

@dukenv0307 @tienifr I tried but it didn't worked, pls try with the image below.

[tienifr]

@Krishna2323 Works fine for me in the sample image in OP (rotated) and also the image you shared

@dukenv0307 can retest and confirm

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[melvin-bot]

This issue has not been updated in over 15 days. @johncschuster, @youssef-lr, @tienifr, @dukenv0307 eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[melvin-bot]

@johncschuster, @youssef-lr, @tienifr, @dukenv0307, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.

[dukenv0307]

I got the delay from @youssef-lr. The PR is still in review, can you please open the issue @johncschuster

[youssef-lr]

Replied in the thread @dukenv0307, I'm still facing issues locally that deal with attachments, hoping we can resolve them today

[tienifr]

We want to close the PR based on this discussion.

[tienifr]

@johncschuster Can you please process payment for me (C) and @dukenv0307 (C+)

[johncschuster]

Sure thing, @tienifr!

I reviewed the issue with @youssef-lr, and it looks like the PR was closed and the fix was not implemented (discussion here). However, we feel you both deserve payment for putting in the work. I'm going to issue 50% of the payment for the work you've done here.

[johncschuster]

Payment Summary:

Contributor: @tienifr paid $250 via Upwork - PAID!
Contributor+: @dukenv0307 owed $250 $500 via Upwork - PAID!

[dukenv0307]

@johncschuster According to the thread, we're closing this because of changing priority (not prioritizing the bug any more), while all the work from contributors were already done.

From this thread to discuss SO on this type of case, the job should be paid out in full.

We should pay in full because we hired them for the job and they did the work they were supposed to.

What do you think?

[johncschuster]

Thanks for raising that, @dukenv0307! I'm happy to issue full payment if this was discussed. Let me change the comment above and also issue the remaining payment to @tienifr.

[dukenv0307]

@johncschuster Also, I receive payment via Upwork, not ND (yet).

Could you pay me via Upwork instead, there's an existing contract.

[johncschuster]

Got it! I'll update that comment above and will pay you now via Upwork.

[johncschuster]

@tienifr, I previously paid you $250 for this job, but I need to issue the remaining $250 given the conversation that @dukenv0307 linked above. Can you please accept the invite to this job?

https://www.upwork.com/nx/wm/offer/104323370

[tienifr]

Can you please accept the invite to this job?

@johncschuster I've just done that, thanks

[johncschuster]

Paid! Thank you!

[johncschuster]

@tienifr / @dukenv0307 / @youssef-lr do we require a regression test for this? If so, can you provide that? If not, we can close this up.

[dukenv0307]

@johncschuster We don't need the regression test since we're closing it because of changing priority

[johncschuster]

Oh duh. I forgot that 🤦 😂

Closing!