Added a fallback icon when getting error on loading image #8852

jayeshmangwani · 2022-05-02T08:26:39Z

Details

Fixed Issues

$ #8291

Tests

Put your phone on airplane mode and turn off wifi
Close the app if it's already open.
Open or reopen the app
If avatar images are not cached then it will fall back on the default

Another way to test

Put your phone on airplane mode and turn off wifi
Close the app if it's already open.
Open or reopen the app
press search from the header and go to search and search the user from the list and check the avatar for user

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Put your phone on airplane mode and turn off wifi
Close the app if it's already open.
Open or reopen the app
If avatar images are not cached then it will fall back on the default

Another way to test

Put your phone on airplane mode and turn off wifi
Close the app if it's already open.
Open or reopen the app
press search from the header and go to search and search the user from the list and check the avatar for user

Verify that no errors appear in the JS console

Screenshots

Web

8291-web.mov

Mobile Web

8291-mWeb.mov

Desktop

8291-Desktop.mov

iOS

8291-ios.mov

Android

8291-Android.1.mp4

shawnborton · 2022-05-02T14:59:46Z

I also wanted to make sure you are addressing this issue here:

Where the avatar circles do not have a default background color set - regardless of whether or not we are able to load them. I think this can easily be fixed by simply adding a background color to the rounded view that wraps each avatar image.

Santhosh-Sellavel · 2022-05-02T16:40:01Z

I also wanted to make sure you are addressing this issue here:

Where the avatar circles do not have a default background color set - regardless of whether or not we are able to load them. I think this can easily be fixed by simply adding a background color to the rounded view that wraps each avatar image.

@jayeshmangwani can you address this too!

Santhosh-Sellavel · 2022-05-02T16:42:20Z

src/components/Icon/Expensicons.js

 import Connect from '../../../assets/images/connect.svg';

 export {
    ActiveRoomAvatar,
    AdminRoomAvatar,
    Android,
    AnnounceRoomAvatar,
+    FallbackAvatar,


Can you move this to alphabetical order, thanks!
i.e it should be added belowExpensifyCard

@Santhosh-Sellavel I have changed the export order for ExpensifyCard and also changed the import order for the same

Santhosh-Sellavel · 2022-05-02T17:01:13Z

src/components/Avatar.js

+    constructor(props) {
+        super(props);
+        this.state = {
+            isErrorFetchingImage: false,


isErrorFetchingImage can we rename it to imageError or showFallbackAvatar ?
@jayeshmangwani
cc: @AndrewGable

replaced isErrorFetchingImage with imageError

…r to avatar

jayeshmangwani · 2022-05-02T18:00:41Z

I also wanted to make sure you are addressing this issue here:

Where the avatar circles do not have a default background color set - regardless of whether or not we are able to load them. I think this can easily be fixed by simply adding a background color to the rounded view that wraps each avatar image.

if we are adding background color to wrapper view then it will look like this screenshot, so I have added a background color to the image, and its working fine , grey color will apear untill image load

Santhosh-Sellavel · 2022-05-02T18:06:07Z

@shawnborton

shawnborton · 2022-05-02T19:09:14Z

Isn't there a view that wraps the avatar image to give it the correct rounded crop? Can we add the background color to them?

jayeshmangwani · 2022-05-02T19:53:22Z

Isn't there a view that wraps the avatar image to give it the correct rounded crop? Can we add the background color to them?

@shawnborton no, we are doing a rounded crop in image styling, let me send you code

App/src/styles/StyleUtils.js

Lines 33 to 40 in 68f0bbf

    
           function getAvatarStyle(size) { 
        
               const avatarSize = getAvatarSize(size); 
        
               return { 
        
                   height: avatarSize, 
        
                   width: avatarSize, 
        
                   borderRadius: avatarSize, 
        
               }; 
        
           }

getAvatarStyle is do the rounded crop and we are using this at Avatar

App/src/components/Avatar.js

Lines 42 to 45 in 68f0bbf

    
           const imageStyle = [ 
        
               StyleUtils.getAvatarStyle(this.props.size), 
        
               ...this.props.imageStyles, 
        
           ];

and imageStyle value using here

App/src/components/Avatar.js

Lines 51 to 53 in 68f0bbf

    
           _.isFunction(this.props.source) 
        
               ? <Icon src={this.props.source} fill={this.props.fill} height={iconSize} width={iconSize} /> 
        
               : <Image source={{uri: this.props.source}} style={imageStyle} />

if you want me to add the background color at getAvatarStyle function then we can do that

shawnborton · 2022-05-02T19:56:20Z

Got it - are we able to set a background color on the image in some kind of way so we don't get a blank area while it loads? I know we can do something like this via CSS where you just give the image a background color and it shows even if the img src doesn't work or hasn't loaded yet. Otherwise we should wrap them so that we can achieve that.

jayeshmangwani · 2022-05-02T20:02:26Z

if I am not wrong then this is already completed, I have added backgroundcolor to the image, and works like this,
the grey color will be visible until the image load

shawnborton · 2022-05-02T20:04:33Z

Okay great! Are you able to show it in action via a screenshot so we can confirm?

jayeshmangwani · 2022-05-02T20:07:00Z

grey-placeholder.mov

Yes sure, please check this video profile picture is taking time to load and default grey background is visible

shawnborton · 2022-05-02T20:08:13Z

Perfect, thank you for sharing the video! Looks good to me.

Santhosh-Sellavel · 2022-05-02T20:47:25Z

All good then!

Santhosh-Sellavel · 2022-05-02T22:10:33Z

I think the avatar has a weird fill here, Is this the expected avatar @shawnborton can you confirm?

shawnborton · 2022-05-02T22:14:42Z

Yeah, I believe the shape inside of the circle should be white.

Santhosh-Sellavel · 2022-05-02T22:18:01Z

@jayeshmangwani Can you check this out there could be some unnecessary fill while showing the fallback avatar thanks!

Santhosh-Sellavel · 2022-05-03T14:09:22Z

src/components/Avatar.js

+                            // eslint-disable-next-line react/jsx-props-no-spreading
+                            {...(!this.state.imageError ? {fill: this.props.fill} : {})}


Why this is messy, can't we simplify it?

Let avoid this unnecessary comments
// eslint-disable-next-line react/jsx-props-no-spreading

should we have to pass fill for the fallback avatar?

What are you trying to do here?

if getting error then not passing the fill prop otherwise fill will be pass

@Santhosh-Sellavel

// eslint-disable-next-line react/jsx-props-no-spreading {...(!this.state.imageError ? {fill: this.props.fill} : {})}

these lines were added for conditional fill prop for Icon component,
Please let me know that do we have to remove the fill prop for the fallback avatar? then I need to add a condition for the Icon component also, here default file is themeColor.icon

App/src/components/Icon/index.js

Lines 47 to 51 in 5f6d8db

<IconToRender

width={width}

height={height}

fill={this.props.fill}

/>

App/src/components/Icon/index.js

Line 33 in 5f6d8db

fill: themeColors.icon,

jayeshmangwani · 2022-05-03T14:10:43Z

@Santhosh-Sellavel added a condition for fill in fallback avtar, even though I am not able repoduce this issue on my side, please check latest code on your side, if issue is still solved or not

Santhosh-Sellavel · 2022-05-03T18:05:24Z

@jayeshmangwani
Sorry for the unnecessary confusion revert the last commit, if needed we can add it later. It seems to be working fine with old changes itself, I'm not sure what went wrong yesterday.

Santhosh-Sellavel · 2022-05-03T18:13:58Z

@jayeshmangwani
Since this Avatar is a generic component used in some other views I tested those components. Seems it affects few view check below

1. Details page

Avatar size is broken

2. In App Settings page Workspace icon

@shawnborton do we have fallback image for workspace icon, or what should be do here.

3. In Workspaces Settings

Size also broken, same as 2

4. Workspace view - General Settings

Size also broken & same as 2

cc: @shawnborton @AndrewGable

shawnborton · 2022-05-03T18:48:41Z

Good catch, let's fix all of those and ideally they all behave the same way - they have a background color applied to fill the space while the image loads, and we use a fallback icon if we can't fetch an image.

For the workspace icon, we could fall back on the default workspace icon (green background with a building) or we could take the same approach where we supply a gray version of the icon? Thoughts?

Santhosh-Sellavel · 2022-05-03T18:58:47Z

A gray version of the icon yes sounds good so that all fallbacks will have the same theme style. But I'm fine with the default green fill with the building also.

shawnborton · 2022-05-03T19:34:34Z

Here is a gray fallback version of the workspace avatar:
fallback-workspace-avatar.svg.zip

…Icon assignment

Santhosh-Sellavel · 2022-05-10T02:02:24Z

Can you resolve those conflicts @jayeshmangwani

jayeshmangwani · 2022-05-10T05:07:48Z

Can you resolve those conflicts @jayeshmangwani

@Santhosh-Sellavel I have merged the latest main to the current branch, and changes have been made that you have suggested except for 1 change, I have commented on the reason for the change let me give you the URL of the change
#8852 (comment)

please let me know what conflicts have to be solved,
Thanks

Santhosh-Sellavel

@jayeshmangwani
Need to update the comment otherwise looks good. Tests well for me.

Also @AndrewGable, can you have a look at this when you have time.

Santhosh-Sellavel · 2022-05-12T21:08:57Z

src/components/Avatar.js

@@ -23,6 +24,9 @@ const propTypes = {

    /** The fill color for the icon. Can be hex, rgb, rgba, or valid react-native named color such as 'red' or 'blue' */
    fill: PropTypes.string,
+
+    /** Function for using fallback avatar */


This doesn’t make any sense, can we add something more meaningful!

@Santhosh-Sellavel does this looks good ?

A fallback avatar component to display when there is an error on loading remote URL

A fallback avatar icon to display when there is an error on loading avatar from remote URL.

@AndrewGable Any thoughts?

jayeshmangwani · 2022-05-16T18:33:39Z

@Santhosh-Sellavel should I update the comment if this comment message is final?
A fallback avatar icon to display when there is an error on loading avatar from remote URL.

Santhosh-Sellavel · 2022-05-16T20:06:54Z

Looks good! @jayeshmangwani

AndrewGable · 2022-05-17T19:19:55Z

@Santhosh-Sellavel - Can you approve? Then I will review! Thanks.

Santhosh-Sellavel · 2022-05-17T19:24:11Z

PR Reviewer Checklist

I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
I verified there are no console errors (if there’s a console error not related to the PR, report it or open an issue for it to be fixed)
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained “why” the code was doing something instead of only explaining “what” the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named “index.js”. All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- Any functional components have the displayName property
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn’t already exist
- The style can’t be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.

Santhosh-Sellavel

Looks good & tests well for me!

OSBotify · 2022-05-17T19:40:07Z

✋ 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-05-18T15:37:33Z

🚀 Deployed to staging by @AndrewGable in version: 1.1.63-0 🚀

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

OSBotify · 2022-05-18T18:38:07Z

🚀 Deployed to staging by @AndrewGable in version: 1.1.63-0 🚀

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

OSBotify · 2022-05-19T17:23:12Z

🚀 Deployed to production by @AndrewGable in version: 1.1.63-2 🚀

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

parasharrajat · 2022-12-08T19:47:44Z

Heads up! This PR was not carefully tested again style issues which caused this #12645

parasharrajat · 2023-01-11T10:05:23Z

Heads up! This bug #12712 was caused due to missing offline tests on this PR.

Santhosh-Sellavel · 2023-01-11T10:23:05Z

Heads up! This bug #12712 was caused due to missing offline tests on this PR.

I'm not sure how this caused that, here we have added only fallback if image was not available, can you brief @parasharrajat so we could understand what exactly went wrong, thanks!

parasharrajat · 2023-01-11T10:33:12Z

So this PR added logic to handle image load failure via this.state.imageError. When the user comes online this state is not reset and the image is never loaded. More information can be present on the tagged issue.

Added a fallback icon when getting error on loading image

5f7f138

jayeshmangwani requested a review from a team as a code owner May 2, 2022 08:26

melvin-bot bot requested review from AndrewGable and Santhosh-Sellavel and removed request for a team May 2, 2022 08:26

Santhosh-Sellavel suggested changes May 2, 2022

View reviewed changes

changed alphabetic order for import and added default background colo…

b78ecd2

…r to avatar

added conditional fill

126598b

Santhosh-Sellavel reviewed May 3, 2022

View reviewed changes

jayeshmangwani added 2 commits May 5, 2022 10:49

changed prop type for fallbackIcon and removed a unnecessary fallback…

d87227e

…Icon assignment

Merge branch 'main' into avatar_fallback_default_when_offline

dced1c4

Merge branch 'main' into avatar_fallback_default_when_offline

d559acb

resolved conflicts

bd03e20

jayeshmangwani requested a review from Santhosh-Sellavel May 11, 2022 16:12

Santhosh-Sellavel suggested changes May 12, 2022

View reviewed changes

updated fallbackIcon comment

e87646d

Santhosh-Sellavel approved these changes May 17, 2022

View reviewed changes

AndrewGable approved these changes May 17, 2022

View reviewed changes

AndrewGable merged commit 9a9e5a5 into Expensify:main May 17, 2022

OSBotify mentioned this pull request May 18, 2022

Deploy Checklist: New Expensify 2022-05-18 #9069

Closed

18 tasks

melvin-bot bot mentioned this pull request May 19, 2022

[HOLD for payment 2022-05-26] [$2000] Avatar images do not reliably fall back on a default when offline #8291

Closed

This was referenced May 24, 2022

[Medium] Chrome+JAWS: Many Pages: Alt is not defined for the Avatars. #8546

Closed

[High]: Chrome+Jaws: Screen reader: Role are not defined for controls. #8418

Closed

parasharrajat mentioned this pull request Dec 8, 2022

[HOLD for payment 2022-12-05] [$250] Log in - Avatar with user's email are very close to each other #12645

Closed

parasharrajat mentioned this pull request Jan 11, 2023

[HOLD for payment 2022-12-20] [$250] Offline default avatar doesn't update after connecting to the internet reported by @thesahindia #12712

Closed

		// eslint-disable-next-line react/jsx-props-no-spreading
		{...(!this.state.imageError ? {fill: this.props.fill} : {})}

	<IconToRender
	width={width}
	height={height}
	fill={this.props.fill}
	/>

Added a fallback icon when getting error on loading image #8852

Added a fallback icon when getting error on loading image #8852

Conversation

jayeshmangwani commented May 2, 2022

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

shawnborton commented May 2, 2022

Santhosh-Sellavel commented May 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayeshmangwani commented May 2, 2022

Santhosh-Sellavel commented May 2, 2022

shawnborton commented May 2, 2022

jayeshmangwani commented May 2, 2022

shawnborton commented May 2, 2022 • edited Loading

jayeshmangwani commented May 2, 2022

shawnborton commented May 2, 2022

jayeshmangwani commented May 2, 2022

shawnborton commented May 2, 2022

Santhosh-Sellavel commented May 2, 2022

Santhosh-Sellavel commented May 2, 2022

shawnborton commented May 2, 2022

Santhosh-Sellavel commented May 2, 2022

Santhosh-Sellavel May 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayeshmangwani commented May 3, 2022

Santhosh-Sellavel commented May 3, 2022

Santhosh-Sellavel commented May 3, 2022 • edited Loading

1. Details page

2. In App Settings page Workspace icon

3. In Workspaces Settings

4. Workspace view - General Settings

shawnborton commented May 3, 2022

Santhosh-Sellavel commented May 3, 2022

shawnborton commented May 3, 2022

Santhosh-Sellavel commented May 10, 2022

jayeshmangwani commented May 10, 2022

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayeshmangwani commented May 16, 2022

Santhosh-Sellavel commented May 16, 2022 • edited Loading

AndrewGable commented May 17, 2022

Santhosh-Sellavel commented May 17, 2022 • edited Loading

PR Reviewer Checklist

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

OSBotify commented May 17, 2022

OSBotify commented May 18, 2022

OSBotify commented May 18, 2022

OSBotify commented May 19, 2022

parasharrajat commented Dec 8, 2022

parasharrajat commented Jan 11, 2023

Santhosh-Sellavel commented Jan 11, 2023 • edited Loading

parasharrajat commented Jan 11, 2023 • edited Loading

shawnborton commented May 2, 2022 •

edited

Loading

Santhosh-Sellavel May 3, 2022 •

edited

Loading

Santhosh-Sellavel commented May 3, 2022 •

edited

Loading

Santhosh-Sellavel commented May 16, 2022 •

edited

Loading

Santhosh-Sellavel commented May 17, 2022 •

edited

Loading

Santhosh-Sellavel commented Jan 11, 2023 •

edited

Loading

parasharrajat commented Jan 11, 2023 •

edited

Loading