Update Fallback Avatar to Support Themes #38674
Conversation
grgia
changed the title
|
@dubielzyk-expensify @s77rt One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
This comment has been minimized.
This comment has been minimized.
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
src/components/Avatar.tsx
Outdated
| const isWorkspace = type === CONST.ICON_TYPE_WORKSPACE; | ||
| const iconSize = StyleUtils.getAvatarSize(size); | ||
|
|
||
| const imageStyle: StyleProp<ImageStyle> = [StyleUtils.getAvatarStyle(size), imageStyles, styles.noBorderRadius]; | ||
| const iconStyle = imageStyles ? [StyleUtils.getAvatarStyle(size), styles.bgTransparent, imageStyles] : undefined; | ||
|
|
||
| const iconFillColor = isWorkspace ? StyleUtils.getDefaultWorkspaceAvatarColor(name).fill : fill; | ||
| // We pass the color styles down to the SVG for the workspace and fallback avatar. | ||
| const useFallBackAvatar = imageError || source === Expensicons.FallbackAvatar; |
There was a problem hiding this comment.
| const useFallBackAvatar = imageError || source === Expensicons.FallbackAvatar; | |
| const useFallBackAvatar = imageError || !source; | |
| const avatarSource = useFallBackAvatar ? fallbackAvatar : source; |
There was a problem hiding this comment.
I agree with this idea, but I'm worried there's a few places in app that are setting the fallback avatar as the avatar. For example here. Maybe we should just return nothing?
There was a problem hiding this comment.
There was a problem hiding this comment.
Seeing as this gets a little hazy, what if I remove this change from the PR (leaving only the theme support) and we handle all the fallback issues in #38743
There was a problem hiding this comment.
Handling this separately sounds good to me 👍
src/libs/OptionsListUtils.ts
Outdated
| userToInvite.icons = [ | ||
| { | ||
| source: UserUtils.getAvatar('', optimisticAccountID), | ||
| source: FallbackAvatar, |
There was a problem hiding this comment.
| source: FallbackAvatar, |
Not specifying any source should by design fallback to the fallback avatart
There was a problem hiding this comment.
I also found setting the source here causes imageError in Avatar in being true
|
Great stuff! There's this very brief flicker that happens between some of the searches. Could we eliminate this? CleanShot.2024-03-25.at.09.28.30.mp4Frozen down I see this: Looks like it's the |
Co-authored-by: Abdelhafidh Belalia <16493223+s77rt@users.noreply.github.com>
|
@s77rt okay, I removed the search changes from this PR and pushed suggestions. I've started a new test build :) |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
|
@dubielzyk-expensify just confirming- should the offline color for the icon look like this? (when a user has an avatar that's not able to load) 
To do this, |
|
When offline we should use our fallback avatar: |
|
@s77rt this is ready for final review |
|
@madmax330 Still discussing unresolved comment #38674 (comment) |
@s77rt fixed
|
|
Rebuilding for easy testing |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/madmax330 in version: 1.4.61-0 🚀
|
|
🚀 Deployed to staging by https://github.com/madmax330 in version: 1.4.61-0 🚀
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 1.4.61-8 🚀
|
Details
This PR updates the fallback avatar to use the same coloring pattern we use for coloring the SVGs for the default workspace avatars
Example of the themes:
Screen.Recording.2024-03-20.at.2.55.46.PM.mov
Also changes the loading color from offline to borders. See #38674 (comment)
This PR only handles the SEARCH Bar, it does not fix this behavior all over the app (ie. chatting new users, manual request options, etc). That will be completed as part of this PR:
#38743
Outdated:
Example of users I know and don't know:
Screen.Recording.2024-03-20.at.1.22.31.PM.mov
If you don't know a user, don't have an account in ONYX, or in any cases where you'd only have an optimisticID, Then we will default to the fallback avatar.
Fixed Issues
$ #33161
#34000
#33470 (may impact testing steps for this one)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop