From 1dfacf9f1ab6a78a4270cc3f232eceecea5c626b Mon Sep 17 00:00:00 2001 From: Mateusz Titz Date: Mon, 10 Jun 2024 15:16:24 +0200 Subject: [PATCH 1/3] Make related code more consistent --- src/components/Avatar.tsx | 12 ++++++------ src/components/AvatarWithIndicator.tsx | 1 + src/components/LHNOptionsList/OptionRowLHN.tsx | 5 +++-- src/components/MenuItem.tsx | 1 + src/components/SubscriptAvatar.tsx | 4 ++-- src/libs/UserUtils.ts | 1 - src/pages/ProfilePage.tsx | 1 + src/pages/ReportParticipantDetailsPage.tsx | 1 + src/pages/workspace/WorkspaceProfilePage.tsx | 1 + src/pages/workspace/WorkspacesListRow.tsx | 1 + .../members/WorkspaceMemberDetailsPage.tsx | 1 + tests/unit/UserUtilsTest.ts | 18 ++++++++++++++++-- 12 files changed, 34 insertions(+), 13 deletions(-) diff --git a/src/components/Avatar.tsx b/src/components/Avatar.tsx index c0d89f4acf1e..07a5fb1f735b 100644 --- a/src/components/Avatar.tsx +++ b/src/components/Avatar.tsx @@ -45,12 +45,12 @@ type AvatarProps = { /** Used to locate fallback icon in end-to-end tests. */ fallbackIconTestID?: string; - /** Denotes whether it is an avatar or a workspace avatar */ - type?: AvatarType; - /** Owner of the avatar. If user, displayName. If workspace, policy name */ name?: string; + /** Denotes whether it is an avatar or a workspace avatar */ + type: AvatarType; + /** Optional account id if it's user avatar or policy id if it's workspace avatar */ avatarID?: number | string; }; @@ -79,10 +79,10 @@ function Avatar({ setImageError(false); }, [originalSource]); - const isWorkspace = type === CONST.ICON_TYPE_WORKSPACE; + const isWorkspace = type === 'workspace'; + const userAccountID = isWorkspace ? undefined : (avatarID as number); - // If it's user avatar then accountID will be a number - const source = isWorkspace ? originalSource : UserUtils.getAvatar(originalSource, avatarID as number); + const source = isWorkspace ? originalSource : UserUtils.getAvatar(originalSource, userAccountID); const useFallBackAvatar = imageError || !source || source === Expensicons.FallbackAvatar; const fallbackAvatar = isWorkspace ? ReportUtils.getDefaultWorkspaceAvatar(name) : fallbackIcon || Expensicons.FallbackAvatar; const fallbackAvatarTestID = isWorkspace ? ReportUtils.getDefaultWorkspaceAvatarTestID(name) : fallbackIconTestID || 'SvgFallbackAvatar Icon'; diff --git a/src/components/AvatarWithIndicator.tsx b/src/components/AvatarWithIndicator.tsx index f024a1239f4e..bf8fe93b8b21 100644 --- a/src/components/AvatarWithIndicator.tsx +++ b/src/components/AvatarWithIndicator.tsx @@ -41,6 +41,7 @@ function AvatarWithIndicator({source, accountID, tooltipText = '', fallbackIcon source={UserUtils.getSmallSizeAvatar(source, accountID)} fallbackIcon={fallbackIcon} avatarID={accountID} + type={CONST.ICON_TYPE_AVATAR} /> diff --git a/src/components/LHNOptionsList/OptionRowLHN.tsx b/src/components/LHNOptionsList/OptionRowLHN.tsx index 851e2a9fdd16..b7b6702ac76e 100644 --- a/src/components/LHNOptionsList/OptionRowLHN.tsx +++ b/src/components/LHNOptionsList/OptionRowLHN.tsx @@ -184,11 +184,12 @@ function OptionRowLHN({reportID, isFocused = false, onSelectRow = () => {}, opti > - {(optionItem.icons?.length ?? 0) > 0 && + {optionItem.icons && + optionItem.icons.length > 0 && (optionItem.shouldShowSubscript ? ( diff --git a/src/components/MenuItem.tsx b/src/components/MenuItem.tsx index 554ae9d4a84a..fb78ff4e39df 100644 --- a/src/components/MenuItem.tsx +++ b/src/components/MenuItem.tsx @@ -561,6 +561,7 @@ function MenuItem( avatarID={avatarID} fallbackIcon={fallbackIcon} size={avatarSize} + type={CONST.ICON_TYPE_AVATAR} /> )} diff --git a/src/components/SubscriptAvatar.tsx b/src/components/SubscriptAvatar.tsx index 46a88c54219c..789b4b4957c3 100644 --- a/src/components/SubscriptAvatar.tsx +++ b/src/components/SubscriptAvatar.tsx @@ -27,8 +27,8 @@ type SubIcon = { }; type SubscriptAvatarProps = { - /** Avatar URL or icon */ - mainAvatar?: IconType; + /** Avatar icon */ + mainAvatar: IconType; /** Subscript avatar URL or icon */ secondaryAvatar?: IconType; diff --git a/src/libs/UserUtils.ts b/src/libs/UserUtils.ts index 946c92fed19d..c5291a2864d7 100644 --- a/src/libs/UserUtils.ts +++ b/src/libs/UserUtils.ts @@ -212,7 +212,6 @@ export { generateAccountID, getAvatar, getAvatarUrl, - getDefaultAvatar, getDefaultAvatarURL, getFullSizeAvatar, getLoginListBrickRoadIndicator, diff --git a/src/pages/ProfilePage.tsx b/src/pages/ProfilePage.tsx index fd2b7696a5b1..df842f633644 100755 --- a/src/pages/ProfilePage.tsx +++ b/src/pages/ProfilePage.tsx @@ -198,6 +198,7 @@ function ProfilePage({route}: ProfilePageProps) { imageStyles={[styles.avatarXLarge]} source={details.avatar} avatarID={accountID} + type={CONST.ICON_TYPE_AVATAR} size={CONST.AVATAR_SIZE.XLARGE} fallbackIcon={fallbackIcon} /> diff --git a/src/pages/ReportParticipantDetailsPage.tsx b/src/pages/ReportParticipantDetailsPage.tsx index 0e22901d8ff7..8f9c101e68cc 100644 --- a/src/pages/ReportParticipantDetailsPage.tsx +++ b/src/pages/ReportParticipantDetailsPage.tsx @@ -87,6 +87,7 @@ function ReportParticipantDetails({personalDetails, report, route}: ReportPartic imageStyles={[styles.avatarXLarge]} source={details.avatar} avatarID={accountID} + type={CONST.ICON_TYPE_AVATAR} size={CONST.AVATAR_SIZE.XLARGE} fallbackIcon={fallbackIcon} /> diff --git a/src/pages/workspace/WorkspaceProfilePage.tsx b/src/pages/workspace/WorkspaceProfilePage.tsx index aabe99019c4f..eb6e687fb337 100644 --- a/src/pages/workspace/WorkspaceProfilePage.tsx +++ b/src/pages/workspace/WorkspaceProfilePage.tsx @@ -141,6 +141,7 @@ function WorkspaceProfilePage({policy, currencyList = {}, route}: WorkSpaceProfi Navigation.navigate(ROUTES.WORKSPACE_AVATAR.getRoute(policy?.id ?? ''))} source={policy?.avatarURL ?? ''} + avatarID={policy?.id} size={CONST.AVATAR_SIZE.XLARGE} avatarStyle={styles.avatarXLarge} enablePreview diff --git a/src/pages/workspace/WorkspacesListRow.tsx b/src/pages/workspace/WorkspacesListRow.tsx index 236ca1e1b89c..5a2462a45cf9 100644 --- a/src/pages/workspace/WorkspacesListRow.tsx +++ b/src/pages/workspace/WorkspacesListRow.tsx @@ -212,6 +212,7 @@ function WorkspacesListRow({ diff --git a/src/pages/workspace/members/WorkspaceMemberDetailsPage.tsx b/src/pages/workspace/members/WorkspaceMemberDetailsPage.tsx index 185b9cd0cfa2..5174e9324523 100644 --- a/src/pages/workspace/members/WorkspaceMemberDetailsPage.tsx +++ b/src/pages/workspace/members/WorkspaceMemberDetailsPage.tsx @@ -160,6 +160,7 @@ function WorkspaceMemberDetailsPage({personalDetails, policy, route}: WorkspaceM imageStyles={[styles.avatarXLarge]} source={details.avatar} avatarID={accountID} + type={CONST.ICON_TYPE_AVATAR} size={CONST.AVATAR_SIZE.XLARGE} fallbackIcon={fallbackIcon} /> diff --git a/tests/unit/UserUtilsTest.ts b/tests/unit/UserUtilsTest.ts index f91f2a499e79..88e91e00f468 100644 --- a/tests/unit/UserUtilsTest.ts +++ b/tests/unit/UserUtilsTest.ts @@ -1,9 +1,23 @@ import * as UserUtils from '@src/libs/UserUtils'; describe('UserUtils', () => { - it('should return the default avatar from the avatar url', () => { + it('should return default avatar if the url is for default avatar', () => { const avatarURL = 'https://d2k5nsl2zxldvw.cloudfront.net/images/avatars/default-avatar_7.png'; - const defaultAvatar = UserUtils.getDefaultAvatar(1, avatarURL); + const defaultAvatar = UserUtils.getAvatar(avatarURL, 1); expect(typeof defaultAvatar).toBe('function'); }); + + it('should return the same url if url is not for default avatar', () => { + const avatarURL = 'https://test.com/images/some_avatar.png'; + const avatar = UserUtils.getAvatar(avatarURL, 1); + expect(avatar).toEqual('https://test.com/images/some_avatar.png'); + }); + + it('should return default avatar if the url is for default avatar', () => { + const avatarURL = 'https://test.com/images/some_avatar.png'; + const defaultAvatar = UserUtils.getAvatar('', 20); + + console.log(defaultAvatar); + expect(defaultAvatar).toEqual('https://test.com/images/some_avatar.png'); + }); }); From 3163173ad4a5cb30e215ca280d1db1516166de5e Mon Sep 17 00:00:00 2001 From: Mateusz Titz Date: Mon, 10 Jun 2024 18:12:58 +0200 Subject: [PATCH 2/3] Fix tests for getAvatar function --- tests/unit/UserUtilsTest.ts | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/tests/unit/UserUtilsTest.ts b/tests/unit/UserUtilsTest.ts index 88e91e00f468..e5378c756751 100644 --- a/tests/unit/UserUtilsTest.ts +++ b/tests/unit/UserUtilsTest.ts @@ -4,20 +4,14 @@ describe('UserUtils', () => { it('should return default avatar if the url is for default avatar', () => { const avatarURL = 'https://d2k5nsl2zxldvw.cloudfront.net/images/avatars/default-avatar_7.png'; const defaultAvatar = UserUtils.getAvatar(avatarURL, 1); + expect(typeof defaultAvatar).toBe('function'); }); it('should return the same url if url is not for default avatar', () => { const avatarURL = 'https://test.com/images/some_avatar.png'; const avatar = UserUtils.getAvatar(avatarURL, 1); - expect(avatar).toEqual('https://test.com/images/some_avatar.png'); - }); - it('should return default avatar if the url is for default avatar', () => { - const avatarURL = 'https://test.com/images/some_avatar.png'; - const defaultAvatar = UserUtils.getAvatar('', 20); - - console.log(defaultAvatar); - expect(defaultAvatar).toEqual('https://test.com/images/some_avatar.png'); + expect(avatar).toEqual('https://test.com/images/some_avatar.png'); }); }); From 6562f7492c669d2ffb74b317ba5d9669c5d61b7a Mon Sep 17 00:00:00 2001 From: Mateusz Titz Date: Wed, 12 Jun 2024 09:27:35 +0200 Subject: [PATCH 3/3] Add fixes to Avatars after review --- src/components/Avatar.tsx | 4 ++-- src/components/LHNOptionsList/OptionRowLHN.tsx | 7 +++---- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/components/Avatar.tsx b/src/components/Avatar.tsx index 07a5fb1f735b..2fbe69a120a0 100644 --- a/src/components/Avatar.tsx +++ b/src/components/Avatar.tsx @@ -64,7 +64,7 @@ function Avatar({ fill, fallbackIcon = Expensicons.FallbackAvatar, fallbackIconTestID = '', - type = CONST.ICON_TYPE_AVATAR, + type, name = '', avatarID, }: AvatarProps) { @@ -79,7 +79,7 @@ function Avatar({ setImageError(false); }, [originalSource]); - const isWorkspace = type === 'workspace'; + const isWorkspace = type === CONST.ICON_TYPE_WORKSPACE; const userAccountID = isWorkspace ? undefined : (avatarID as number); const source = isWorkspace ? originalSource : UserUtils.getAvatar(originalSource, userAccountID); diff --git a/src/components/LHNOptionsList/OptionRowLHN.tsx b/src/components/LHNOptionsList/OptionRowLHN.tsx index b7b6702ac76e..66b444daf33a 100644 --- a/src/components/LHNOptionsList/OptionRowLHN.tsx +++ b/src/components/LHNOptionsList/OptionRowLHN.tsx @@ -184,18 +184,17 @@ function OptionRowLHN({reportID, isFocused = false, onSelectRow = () => {}, opti > - {optionItem.icons && - optionItem.icons.length > 0 && + {!!optionItem.icons?.length && (optionItem.shouldShowSubscript ? ( ) : (