feat: add image loading placeholder #7723

mdneyazahmad · 2022-02-13T19:00:18Z

Details

Add a loading indicator while loading an image.

Fixed Issues

$ #7584

Tests

Open a chat with images
Verify that a loading indicator appears for images that are loading

Verify that no errors appear in the JS console

QA Steps

Same as test steps

Verify that no errors appear in the JS console

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

mountiny

Bump for lint error, thank you for fixing it. Gonna wait for Rushat for when he gets time to have a look.

mountiny · 2022-02-13T20:05:42Z

src/components/ImageWithSizeCalculation.js

+
+        this.state = {
+            loading: true,
+        }


@mdneyazahmad Thank you for the PR! Got one styling mistake here.

Fixed the linting error

Thanks for the bump Vit :)

rushatgabhane · 2022-02-14T07:02:10Z

@mdneyazahmad there's a regression - loading indicator isn't show when image is being uploaded.
Let's fix this 💪

Screencast.from.14-02-22.09-57-05.AM.+03.mp4

mdneyazahmad · 2022-02-14T07:24:44Z

@rushatgabhane could you retest? It seems to work as expected.

test2.mp4

mdneyazahmad · 2022-02-14T07:26:43Z

Also after this pr, dimension jumping issue is even more highlighted. That does not look good.

rushatgabhane · 2022-02-14T07:35:32Z

could you retest? It seems to work as expected.

@mdneyazahmad hmm i can still repro it after hard refresh, and in incognito mode.

Can you try disabling your cache from chrome dev tools -> network tab -> disable cache

mdneyazahmad · 2022-02-14T07:39:55Z

@rushatgabhane I did the same it still works.

mdneyazahmad · 2022-02-14T07:43:24Z

Could you please do npm install and then npm run web?

rushatgabhane · 2022-02-14T17:11:12Z

@mdneyazahmad nope that didn't help.

@mountiny could you please help us verify this whenever you can, thanks!

mountiny · 2022-02-15T12:24:48Z

Sorry for the delay, I will be able to have a look into this later today or tomorrow the latest.

mountiny · 2022-02-15T17:13:22Z

@mdneyazahmad Hmm, weird, I am getting the same problem as Rushat 😬
https://user-images.githubusercontent.com/36083550/154113512-1e0c9aa9-3bf3-4884-9716-b5442ffd9d39.mov

mdneyazahmad · 2022-02-16T02:28:58Z

I tested in (Chrome/Firefox/Safari), it works on all these browser.

@mdneyazahmad there's a regression - loading indicator isn't show when image is being uploaded.
Let's fix this 💪

Here is the issue with this feature. #7463

I am not sure why. Could you please try with merging main (if not). If still does not work, I will have to find the root cause for it.

@rushatgabhane

mountiny · 2022-02-16T12:09:33Z

@mdneyazahmad Tested again with merged main in Chrome and the same result. This time the blank placeholder disappeared, then the spinner showed up and then the picture showed up. Lots of jumping, not a great experience. I am not sure what is going on there.

Also I got confused with the linked issue since it looks almost the same. If I understand the difference, this PR should focus on spinner for the image being loaded in chat if not cached, the other one for when it is uploaded.

Either way it seems like it is not working as expected right now, but I am not sure how to help with debugging now, odd you cant reproduce the problem since it happens everytime for me.

mountiny · 2022-02-16T12:11:03Z

I am using local set up which might influence the way/speed the upload is happening but I think Rushat is using the same setup as you do.

rushatgabhane · 2022-02-16T12:51:38Z

Yep, I'm using local setup and it happens everytime.

mountiny · 2022-02-20T16:11:58Z

@mdneyazahmad Any progress on this one? Have you been able to replicate it?

okansahin · 2022-02-22T09:54:08Z

Hello guys,

Sorry I saw this issue recently and no one mentioned me too. I wish you to mention me in here.

First of all, jumping problem is looks bad. But it is already at there before my PR. So there is no relation with the code I sent in PR. It is a different issue.

As for the problem some of you are having here, it stands before your eyes :)

It's just about @mdneyazahmad uploads by clicking add attachment button, @rushatgabhane and @mountiny upload by dragging. I fixed this and sent to related branch.

Please try to reproduce it with drag&drop before getting branch to confirm me.

After I fixed it:
okansahin@63213b0

Screen.Recording.2022-02-22.at.13.54.19.mov

Thank you

mdneyazahmad · 2022-02-22T11:52:50Z

Thanks @okansahin for figuring it out. I was able to reproduce it with drag and drop.
@rushatgabhane and @mountiny Could you confirm that it works when add attachment button is clicked and uploaded?

rushatgabhane · 2022-02-22T12:39:13Z

src/components/ImageWithSizeCalculation.js

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


@mdneyazahmad let's rename it to isLoading
https://github.com/Expensify/App/blob/9ef244cf828f56d31c033df185f1eda4d2c0db28/STYLE.md#boolean-variables-and-props

@rushatgabhane done

rushatgabhane

@mdneyazahmad for the jumping issue.
We could reduce the number of jumps from 2 to 1, if we use the same thumbnail styles as the upload preview thumbnail.

What do you think? Can we do any better?

Screencast.from.22-02-22.04-38-40.PM.+03.mp4

mountiny · 2022-02-22T16:19:58Z

@rushatgabhane how many jumps are there without this PR? If there is just one and this PR changes it to 2, then we should fix it here.

But if there already were 2 jumps before this PR, we should keep that in a separate job just to be atomic with the issues. Feel free to report the issue/bug!

mountiny · 2022-02-22T16:22:36Z

@okansahin Aaand thank you very much for this of course! Really appreciate your help 🙇

rushatgabhane · 2022-02-22T16:24:29Z

There are 2 jumps without this PR.

we should keep that in a separate job just to be atomic with the issues.

Sure! That makes sense, thanks for jumping in :)

rushatgabhane

@mountiny LGTM! 🎉 Tests well on all platforms.

Kudos to @okansahin for connecting the dots 🙌
I've commented on #7463 to get the upload preview fixed

okansahin · 2022-02-23T08:34:24Z

Thanks to you too, all. 🙌

mountiny

Nice! Thank you @mdneyazahmad for the patience and work on this PR. thanks to @okansahin for help as well and great job @rushatgabhane testing and reviewing it!

OSBotify · 2022-02-23T15:26:29Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2022-02-23T21:59:00Z

🚀 Deployed to staging by @mountiny in version: 1.1.40-0 🚀

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

roryabraham · 2022-02-28T20:19:09Z

This PR was confirmed to cause a regression, and has been reverted.

OSBotify · 2022-03-01T18:49:19Z

🚀 Deployed to production by @yuwenmemon in version: 1.1.40-2 🚀

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

Stutikuls · 2022-03-09T06:48:49Z

Issue 1 -

Title- [Medium]: Chrome+ Jaws : Screen reader : Name & Role is not defined for 'Actions (+)' control.
Actual - Screen reader is reading some other information on Actions(+) control.
Expected - Role = 'Button' and Name = 'Actions/Add attachments' should defined for Actions(+) control.
WCAG failure - 4.1.2
Reproducible in staging?: - Yes
Version Number : - v1.1.40-2
Platforms - Web (Chrome + Jaws), iOS, Desktop (MAC)

7723_NAme.role.is.not.defined.for.Action.+.control.mp4

Issue 2 -

Title- [Medium]: Chrome+ Jaws : Keyboard : 'Actions (+)' control is not accessible using screen reader.
Actual - 'Actions (+)' control is not accessible using screen reader, while press Enter on the control focus moves to another control.
Expected - Actions (+)' control should accessible using screen reader.
WCAG failure - 2.1.1
Reproducible in staging?: - Yes
Version Number: - v1.1.40-2
Platforms - Web (Chrome + Jaws), iOS, Mobile-web(iOS)

7723_Actions.+.control.is.not.accessible.using.screen.reader.mp4

Issue 3 -

Title- [Medium]: Chrome+ Jaws : Screen reader: Role is not defined for 'Send' control.
Actual - 'Actions (+)' control is not accessible using screen reader, while press Enter on the control focus moves to another control.
Note- On MAC screen reader is reading "Send Group".
Expected - Actions (+)' control should accessible using screen reader.
WCAG failure - 4.1.2
Reproducible in staging?: - Yes
Version Number: - v1.1.40-2
Platforms - Web (Chrome + Jaws), iOS, Mobile-web (iOS), Desktop(MAC)

7723_Role.is.not.defined.for.Send.control.mp4

Issue 4 -

Title- [Medium]: Chrome+ Jaws : Screen reader: Status message is not being announced by screen reader after activate the 'Send' control.
Actual - Status message is not being announced by screen reader after activate the 'Send' control. Screen reader is silent and focus lands on the Action(+) button.
Expected - Status message should announce by screen reader after uploading the attachment. Screen reader should read like "Image/video has been uploaded".
WCAG failure - 4.1.3
Reproducible in staging?: - Yes
Version Number: - v1.1.40-2
Platforms - Web (Chrome + Jaws), iOS, Mobile-web (iOS), Desktop(MAC). Android

7723_SCreen.reader.is.not.provide.the.information.after.sent.the.attachement.mp4

feat: add image loading placeholder

6ec539d

mdneyazahmad requested a review from a team as a code owner February 13, 2022 19:00

MelvinBot requested review from mountiny and rushatgabhane and removed request for a team February 13, 2022 19:00

mountiny reviewed Feb 13, 2022

View reviewed changes

style: add semi-colon

4791a24

Merge branch 'main' into feat/7584-image-placeholder

d55e5ca

okansahin added a commit to okansahin/App that referenced this pull request Feb 22, 2022

Fix for Expensify#7723

63213b0

rushatgabhane reviewed Feb 22, 2022

View reviewed changes

rushatgabhane mentioned this pull request Feb 22, 2022

[HOLD for payment 2022-03-17] [HOLD for payment 2022-03-09] Feature Request: Add image preview placeholder while uploading images/Videos - Reported by @mdneyazahmad #7463

Closed

refactor: rename variable names

73c5eaa

rushatgabhane suggested changes Feb 22, 2022

View reviewed changes

rushatgabhane approved these changes Feb 22, 2022

View reviewed changes

okansahin mentioned this pull request Feb 23, 2022

Upload image preview placeholder - Follow-up #7569 #7874

Merged

6 tasks

mountiny approved these changes Feb 23, 2022

View reviewed changes

mountiny merged commit cc90f56 into Expensify:main Feb 23, 2022

OSBotify mentioned this pull request Feb 23, 2022

Deploy Checklist: New Expensify 2022-02-23 #7883

Closed

69 tasks

kidroca mentioned this pull request Feb 24, 2022

IOS - Attachment - Image upload remains loading #7900

Closed

mountiny mentioned this pull request Feb 28, 2022

Revert "feat: add image loading placeholder" #7933

Merged

roryabraham mentioned this pull request Feb 28, 2022

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

Closed

SofiedeVreese mentioned this pull request Mar 6, 2022

[HOLD for payment 2022-04-13] When clicking on chat open Image viewer. It does not show a loading indicator - reported by @mdneyazahmad #7905

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add image loading placeholder #7723

feat: add image loading placeholder #7723

mdneyazahmad commented Feb 13, 2022

mountiny left a comment

mountiny Feb 13, 2022

mdneyazahmad Feb 14, 2022

rushatgabhane Feb 14, 2022

rushatgabhane commented Feb 14, 2022

mdneyazahmad commented Feb 14, 2022

mdneyazahmad commented Feb 14, 2022

rushatgabhane commented Feb 14, 2022 •

edited

Loading

mdneyazahmad commented Feb 14, 2022

mdneyazahmad commented Feb 14, 2022

rushatgabhane commented Feb 14, 2022

mountiny commented Feb 15, 2022

mountiny commented Feb 15, 2022

mdneyazahmad commented Feb 16, 2022

mountiny commented Feb 16, 2022

mountiny commented Feb 16, 2022

rushatgabhane commented Feb 16, 2022 •

edited

Loading

mountiny commented Feb 20, 2022

okansahin commented Feb 22, 2022 •

edited

Loading

mdneyazahmad commented Feb 22, 2022

rushatgabhane Feb 22, 2022

mdneyazahmad Feb 22, 2022

rushatgabhane left a comment •

edited

Loading

mountiny commented Feb 22, 2022

mountiny commented Feb 22, 2022

rushatgabhane commented Feb 22, 2022

rushatgabhane left a comment •

edited

Loading

okansahin commented Feb 23, 2022

mountiny left a comment

OSBotify commented Feb 23, 2022

OSBotify commented Feb 23, 2022

roryabraham commented Feb 28, 2022

OSBotify commented Mar 1, 2022

Stutikuls commented Mar 9, 2022 •

edited

Loading

feat: add image loading placeholder #7723

feat: add image loading placeholder #7723

Conversation

mdneyazahmad commented Feb 13, 2022

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

mountiny left a comment

Choose a reason for hiding this comment

mountiny Feb 13, 2022

Choose a reason for hiding this comment

mdneyazahmad Feb 14, 2022

Choose a reason for hiding this comment

rushatgabhane Feb 14, 2022

Choose a reason for hiding this comment

rushatgabhane commented Feb 14, 2022

mdneyazahmad commented Feb 14, 2022

mdneyazahmad commented Feb 14, 2022

rushatgabhane commented Feb 14, 2022 • edited Loading

mdneyazahmad commented Feb 14, 2022

mdneyazahmad commented Feb 14, 2022

rushatgabhane commented Feb 14, 2022

mountiny commented Feb 15, 2022

mountiny commented Feb 15, 2022

mdneyazahmad commented Feb 16, 2022

mountiny commented Feb 16, 2022

mountiny commented Feb 16, 2022

rushatgabhane commented Feb 16, 2022 • edited Loading

mountiny commented Feb 20, 2022

okansahin commented Feb 22, 2022 • edited Loading

mdneyazahmad commented Feb 22, 2022

rushatgabhane Feb 22, 2022

Choose a reason for hiding this comment

mdneyazahmad Feb 22, 2022

Choose a reason for hiding this comment

rushatgabhane left a comment • edited Loading

Choose a reason for hiding this comment

mountiny commented Feb 22, 2022

mountiny commented Feb 22, 2022

rushatgabhane commented Feb 22, 2022

rushatgabhane left a comment • edited Loading

Choose a reason for hiding this comment

okansahin commented Feb 23, 2022

mountiny left a comment

Choose a reason for hiding this comment

OSBotify commented Feb 23, 2022

OSBotify commented Feb 23, 2022

roryabraham commented Feb 28, 2022

OSBotify commented Mar 1, 2022

Stutikuls commented Mar 9, 2022 • edited Loading

rushatgabhane commented Feb 14, 2022 •

edited

Loading

rushatgabhane commented Feb 16, 2022 •

edited

Loading

okansahin commented Feb 22, 2022 •

edited

Loading

rushatgabhane left a comment •

edited

Loading

rushatgabhane left a comment •

edited

Loading

Stutikuls commented Mar 9, 2022 •

edited

Loading