[HOLD for payment 2022-04-13] Feature Request: Add support for an image placeholder in chat - Reported by @mdneyazahmad

[mvtglobally]

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 app
2. Load big size image or any image on the slower network

Expected Result:

Image placeholder should display. Something like a loading spinner

Actual Result:

Blank square displayed

Workaround:

unknown

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number: 1.1.36-0
Reproducible in staging?: Y
Reproducible in production?: Y
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

image-placeholder.mp4

Expensify/Expensify Issue URL:
Issue reported by: @mdneyazahmad
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1643212660496300

View all open jobs on GitHub

[MelvinBot]

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

[MelvinBot]

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

[mdneyazahmad]

Proposal

Proposing as I reported this issue

We need to add a loading state to track when the image is loaded. Image component accepts a prop onLoadEnd which is fired when the image is successfully loaded or it has failed. We set loading to false when it is fired. All changes are made in file src/components/ImageWithSizeCalculation.js.

+    constructor(props) {
+        super(props);
+
+        this.state = {
+            loading: true,
+        }
+    }

Now, conditionally display the loading indicator.

-import {Image} from 'react-native';
+import {Image, ActivityIndicator} from 'react-native';

+import themeColors from '../styles/themes/default'
 
     render() {
         return (
+            <>
                 <Image
                     style={[
                         styles.w100,
                         styles.h100,
+                        this.state.loading && styles.dNone,
                         this.props.style,
                     ]}
                     source={{uri: this.props.url}}
                     resizeMode="contain"
+                    onLoadEnd={() => this.setState({loading: false})}
+                />
+                {this.state.loading && (
+                    <ActivityIndicator
+                        size="large"
+                        color={themeColors.textSupporting}
+                        style={[styles.flex1]}
+                    />
+                )}
+            </>
         );
     }
 }

Result

Screen.Recording.2022-02-06.at.4.58.45.PM.mov

[MelvinBot]

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

[MelvinBot]

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

[mallenexpensify]

Job posted https://www.upwork.com/jobs/~018c2e439c0f0663e4
@rushatgabhane can you review @mdneyazahmad 's proposed solution above? (or.. if you're out, unassign yourself and ping @mountiny to ask to review)
#7584 (comment)

[rushatgabhane]

@mdneyazahmad I was thinking of keeping the dimensions of loading box same as the image, so it doesn't resize. But I don't think we can do that before the image has loaded.

Nevermind, this was discussed on slack.

[rushatgabhane]

@mdneyazahmad what's styles.dNone and why do we need it?

[mdneyazahmad]

If we conditionally render the image then, Image will not be mounted and hence onLoadEnd will never be fired. styles.dNone will make it disappears but it will be there.

[rushatgabhane]

@mdneyazahmad I understand.
By styles.dNone you mean display: none?
Because I can't find dNone anywhere

[mdneyazahmad]

Yes, It is display: none. Sorry, for misunderstanding.

App/src/styles/utilities/display.js

Lines 11 to 23 in 78d6e68

	export default {
	dFlex: {
	display: 'flex',
	},
	dNone: {
	display: 'none',
	},
	dInline: {
	display: 'inline',
	},
	};

[rushatgabhane]

🎀👀️🎀 C+ reviewed

@mountiny I like @mdneyazahmad's proposal.

[mallenexpensify]

@mdneyazahmad can you apply to Upwork job here? https://www.upwork.com/jobs/~01c689e49c8567de30
I had to create a new job, the other one was expired

[mountiny]

Hi @mdneyazahmad, just wanted to bump this one, I realize it has not been the easiest of problems to solve for you since you were unable to reproduce the regression, however, as you are active on other issues, I would like to point out that it is expected that once you are assigned to some issue to solve it, that issue should be your priority over making proposals to other jobs.

I hope all is good and that we can push this one along in the coming days 🙏 Let us know here if you have any problems or something time-sensitive going on. We understand we are not pedantic over here, but clear and frequent communication is key :)

Thank you very much!

[mdneyazahmad]

@mountiny I am sorry for being late here. I was creating a pr and I got issue with mobile web (testing) in my machine. I just don't know why Safari (ios simulator) can not open localhost:8080 I can still open localhost:9000 but that is the old one. I also asked on Slack if somebody can help https://expensify.slack.com/archives/C01GTK53T8Q/p1647430519293939

[mountiny]

Thanks for the message. That is alright and I am sorry you are having problems with your setup. Ideally, try to write as accurately as possible the steps you have taken to run it and what errors/logs you see in Slack so others can try to reproduce. Please, try to keep the communication more active.

Thank you for your efforts here!

[mdneyazahmad]

@mountiny I posted the steps with a video.

[mountiny]

Reverted the PR for precautionary measures as a performance issue has been found. Work will be continued in a new PR #8238

[mallenexpensify]

Can someone provide an update on where we're at with this issue? What's the best PR link to be following/checking?

[rushatgabhane]

@mallenexpensify #8238 is the PR to follow

Looks like it wasn't properly linked. cc: @mdneyazahmad

[botify]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.46-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• [Feat] Add Image loading indicator #8203

If no regressions arise, payment will be issued on 2022-04-06. 🎊

[mountiny]

Updated the title and everything, the PR we are following is linked fine but previous PR which was reverted triggered this update, we are about to merge the follow up

[mallenexpensify]

This is ready to be paid, right?
@rushatgabhane $250 for C+
@mdneyazahmad $500 - $250/each for reporting and fix.

Please apply here https://www.upwork.com/jobs/~01c689e49c8567de30

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.51-0 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• [Feat] Add image loading indicator #8238

If no regressions arise, payment will be issued on 2022-04-13. 🎊

[mdneyazahmad]

@mallenexpensify applied

[mallenexpensify]

Hired both in Upwork, let me know when you've accepted the offers. @mdneyazahmad let me know here if you're not "Neyaz A." Also...the job price is $250, you'll be bonused the other $250 owed.

[mdneyazahmad]

@mallenexpensify accepted the offer Thank you!

[mallenexpensify]

Paid @rushatgabhane and @mdneyazahmad one day early cuz I was paying another issue. $250 each for C+ and fix