Add New Default Workspace Avatars by grgia · Pull Request #15280 · Expensify/App

grgia · 2023-02-19T00:37:19Z

Details

We are adding the new default workspace avatars (SVGs) as part of the new branding changes.

The default workspace avatar is dynamically created by determining the first appearing letter/number of the workspace title. If none, then the workspace defaults to the building icon. For example, Expensify would result in an E, !Expensify would result in an E, and ! would result in a building.

Similarly, the title is used to randomly select a color pairing. Because default avatars are not currently stored in memory, this ensures that the default avatar colors remain consistent. The title is stripped from any trailing whitespace. Therefore, Expensify will have a different color than Expensify 1 and so on.

Whenever an avatar is used, we need to know three things in order to properly style it:

Is this a workspace avatar or a user avatar? (square, circle)
Who is this avatar for? (user/login, workspace/name)
What is the source URL of this avatar, if any? (user-uploaded, none)

I refactored <Avatar> to take in this information (type, name, source)
This will mean <Avatar> will be able to own the default avatar calculations. This also means that we can later support more types of avatars if needed (rooms, etc).

I will be following up this PR with another PR to clean up the default avatars for users.

Current Branch:

Screen.Recording.2023-02-18.at.4.42.05.PM.mov

Fixed Issues

$ #12259

Tests

Verify that no errors appear in the JS console
Verified that A-Z/0-9 and building SVG all work by changing the title of the workspace
Verified that changing the title (but not the first letter) of the workspace results in a color change
Verified that using a user-uploaded workspace avatar works
Completed QA steps below.

Offline tests

QA Steps

Verify that no errors appear in the JS console
Create 5 Workspaces
Add a user-uploaded avatar to two of them, and ensure that it looks correct (square border) on all pages.
Remove the user-uploaded avatar from one, and ensure that it now uses a default avatar.
For the other 4, name them with different first letters. Ensure that they get a correct SVG avatar on all pages.
Add a member to each workspace.

Repeat this with both the user-uploaded workspace and the normal workspace:

In a Workspace Chat (header, large icon)
In the LHN.
In the Search Bar http://localhost:8080/search
In the settings page - http://localhost:8080/settings
On the workspaces page - http://localhost:8080/settings/workspaces
On a single workspaces space - http://localhost:8080/workspace/{workspace ID}
On a single workspaces settings - http://localhost:8080/workspace/{workspace ID}/settings

Because this modified the Avatar Component, we will also check that user avatars look correct. Make sure you have at least 1 avatar that uses a user-uploaded avatar to reference before checking these pages:

PR Author Checklist

Screenshots/Videos

Web

Screen.Recording.2023-02-21.at.11.06.04.AM.mov

Mobile Web - Chrome

Screen.Recording.2023-02-21.at.11.09.33.AM.mov

Mobile Web - Safari

Screen.Recording.2023-02-21.at.11.06.56.AM.mov

Desktop

Screen.Recording.2023-02-21.at.11.12.58.AM.mov

iOS

Screen.Recording.2023-02-21.at.11.08.40.AM.mov

Android

Screen.Recording.2023-02-21.at.11.10.41.AM.mov

…kspace avatars

…ions

shawnborton · 2023-02-20T15:43:36Z

This might be my fault but looks like the default workspace avatar is missing one of the three lines to make it look like a building:

Let me know if you need a new svg for that.

shawnborton · 2023-02-20T15:44:38Z

Otherwise this is looking dope!

MelvinBot · 2023-02-21T20:12:27Z

@mananjadhav @aldo-expensify 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]

mananjadhav · 2023-02-22T12:48:40Z

    fill: themeColors.icon,
    small: false,
    inline: false,
-    isSVGAvatar: false,


qq, I remember we added this in the last PR, just to share some knowledge, why we removed it now?

Rather than have the Icon component handle avatar styles, I decided that the Avatar component should own all things avatars. Therefore, I replaced the styling with additionalStyles and pass in the correct avatar styles from within the component instead.

That said, I will remove isWorkspaceAvatar too!

mananjadhav · 2023-02-22T12:58:40Z


    /** Flag to choose between avatar image or an icon */
-    iconType: PropTypes.oneOf([CONST.ICON_TYPE_AVATAR, CONST.ICON_TYPE_ICON]),
+    iconType: PropTypes.oneOf([CONST.ICON_TYPE_AVATAR, CONST.ICON_TYPE_ICON, CONST.ICON_TYPE_WORKSPACE]),


nab, or probably an overkill. Do we want to create this as a prop in avatarPropTypes? and use that instead of importing this everywhere?

I think no, but only because we can have different iconTypes:
MenuItem can take in:

[CONST.ICON_TYPE_AVATAR, CONST.ICON_TYPE_ICON, CONST.ICON_TYPE_WORKSPACE]

Whereas Avatars take in:

[CONST.ICON_TYPE_AVATAR, CONST.ICON_TYPE_WORKSPACE]

Although one option would be that we could make separate propTypes for these. Like MenuItemIconTypes and AvatarIconTypes. What are your thoughts given that?

mananjadhav

Did one round of review, and raised some comments. I think I'll need one more review as there are 20+ files to review.

aldo-expensify · 2023-03-03T00:11:39Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
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 included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
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 is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review 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 other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
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 */
- 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 any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
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.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Screen.Recording.2023-03-02.at.4.57.58.PM.mov

Mobile Web - Chrome

Screen.Recording.2023-03-02.at.5.01.45.PM.mov

Mobile Web - Safari

Screen.Recording.2023-03-02.at.6.03.38.PM.mov

Desktop

Screen.Recording.2023-03-02.at.6.05.59.PM.mov

iOS

Screen.Recording.2023-03-02.at.5.53.32.PM.mov

Android

aldo-expensify · 2023-03-03T01:07:24Z

@grgia , when there are more than 4 WPs, you can still see the 4th workspace avatar plus the +count on top of it:

I other cases, you don't show the avatar anymore, just the +count which makes it more readable. For example:

Should we apply the same technique?

cc @shawnborton

Edit: I see now that avatar is darker when the +count is on top, so maybe this is how we designed it.

aldo-expensify · 2023-03-03T01:59:52Z

I'm wondering if a trashed workspace should have a square icon preserving the shape of workspace icons:

If it makes sense, I'm ok with leaving it for a follow up PR to avoid delaying this more.

aldo-expensify

Looks good and tested well for me.

If if if we decide to do something about this two comments:

#15280 (comment)
#15280 (comment)

We can do it in a follow up PR, no need to hold the PR on that.

Going to leave unmerged, in case you want to wait for @rushatgabhane to have a look. Feel free to merge when you think is appropriate!

grgia · 2023-03-03T03:07:43Z

I'm going to go ahead and merge this one to avoid conflicts!

I made two issues for the above comments that I'll fix in a follow up PR:
#15635
#15636

mananjadhav · 2023-03-03T03:13:19Z

I tested on web and desktop. I’ll finish the checklist in an hour and raise if any issues. Can be picked as a follow up PR.

OSBotify · 2023-03-03T03:14:16Z

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

mananjadhav · 2023-03-03T05:32:11Z

@grgia @shawnborton Here are different cases for the workspace title

If you notice *hello and ÆTest, the first characters are ignored and the workspace avatar picks from the second symbol. Is that the expected output? or does it needs to be fixed?

mananjadhav · 2023-03-03T05:47:22Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
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 included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
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 is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review 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 other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
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 */
- 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 any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
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.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

web-workspace-avatar.mov

Mobile Web - Chrome

Mobile Web - Safari

Desktop

desktop-workspace-avatar.mov

iOS

Screen.Recording.2023-03-03.at.11.10.54.AM.mov

Android

shawnborton · 2023-03-03T12:35:49Z

I other cases, you don't show the avatar anymore, just the +count which makes it more readable. For example: Should we apply the same technique?

I don't think so, no. If anything, we can bump down the opacity of the overlay. But this style matches what it looks like when you start a DM with multiple people:

(Also wow, I did not realize Concierge would send a message when I created that test group 😨 )

Anyways, thank you Georgia for pushing this through, it's beautiful!!

shawnborton · 2023-03-03T12:37:42Z

@mananjadhav for your comment here, I think this is okay for now. Let's just be diligent about this when the inevitable bug reports come in. But yeah, I don't think we're going to have actual real workspaces come in with these kinds of characters just yet, so let's make people aware that we are purposely choosing not to address this for the time being.

OSBotify · 2023-03-06T16:43:53Z

🚀 Deployed to staging by https://github.com/grgia in version: 1.2.79-0 🚀

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

OSBotify · 2023-03-07T17:21:00Z

🚀 Deployed to production by https://github.com/roryabraham in version: 1.2.79-0 🚀

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

s77rt · 2023-03-27T16:43:22Z

This PR introduced a regression here #15697 due to missing offline tests.

arosiclair · 2023-04-04T14:45:46Z

Hey 👋🏾, it seems like we first implemented square workspace icons with this PR. However, the shape of the mask we use for cropping uploaded workspace avatars wasn't updated from a circle to a square as well which resulted in this bug.

narefyev91 · 2023-10-16T07:23:24Z

-                {_.map(iconsToDisplay, (val, index) => (
-                    <View key={`${val}${index}`} style={[styles.justifyContentCenter, styles.alignItemsCenter]}>
+                {_.map(iconsToDisplay, (icon, index) => (
+                    <View key={`${icon.source}${index}`} style={[styles.justifyContentCenter, styles.alignItemsCenter]}>


Using source as key cause a regression #28074

Thanks for the note @narefyev91.

eVoloshchak · 2023-11-02T18:11:58Z

-                                icons: [ReportUtils.getAvatar(item.avatar, item.login)],
+                                icons: [{
+                                    source: ReportUtils.getAvatar(item.avatar, item.login),
+                                    name: item.login,


Coming from #29645
Not sure if this issue was present at the time this PR was created, but we should have used formatPhoneNumber from withLocalize

name: props.formatPhoneNumber(details.login),

grgia added 11 commits February 3, 2023 14:35

Use hash of policy name to determine color combo, use new default wor…

5f8e076

…kspace avatars

Add new workspace SVGs / import them

27c7ed7

Fix range for color options

8107cad

Merge branch 'main' into georgia-new-workspace-avatars

e03099a

Restructure icons shape in order to handle workspace avatars everywhere

0a96372

Add default building for non A-Z/0-9 characters, clean up utils funct…

b541a99

…ions

Move proptypes into new file

d9143df

Use proptype for floatRightAvatars

6f90cb3

Styling for settings page horizontal avatars

e156dee

refactor getAvatarBorderStyle

5428178

Removed isWorkspaceAvatar prop

9823350

grgia self-assigned this Feb 19, 2023

grgia requested a review from shawnborton February 19, 2023 00:44

grgia added 2 commits February 20, 2023 10:35

Fix building SVG

e54ca2a

Fix SVG fill on mobile

3ea122a

grgia marked this pull request as ready for review February 21, 2023 20:11

grgia requested a review from a team as a code owner February 21, 2023 20:11

grgia changed the title ~~[WIP] Add New Default Workspace Avatars~~ Add New Default Workspace Avatars Feb 21, 2023

melvin-bot Bot requested review from aldo-expensify and mananjadhav and removed request for a team February 21, 2023 20:12

mananjadhav reviewed Feb 22, 2023

View reviewed changes

Comment thread src/components/Icon/index.js Outdated

mananjadhav reviewed Feb 22, 2023

View reviewed changes

Comment thread src/components/MultipleAvatars.js

mananjadhav reviewed Feb 22, 2023

View reviewed changes

Comment thread src/components/RoomHeaderAvatars.js Outdated

mananjadhav reviewed Feb 22, 2023

View reviewed changes

grgia added 2 commits March 2, 2023 16:31

update workspaceColorOptions to use objects, update outdated comments

9f9c47f

rename color to fallbackIconColor

b48c2f0

grgia requested a review from aldo-expensify March 3, 2023 00:45

aldo-expensify approved these changes Mar 3, 2023

View reviewed changes

This was referenced Mar 3, 2023

Update Workspace Icon for 4+ Workspaces #15635

Closed

Let's update the trash icon for deleted workspaces to match workspace styling #15636

Closed

grgia merged commit 9953eb8 into main Mar 3, 2023

grgia deleted the georgia-new-workspace-avatars branch March 3, 2023 03:07

OSBotify mentioned this pull request Mar 6, 2023

Deploy Checklist: New Expensify 2023-03-06 #15684

Closed

17 tasks

MelvinBot mentioned this pull request Mar 7, 2023

[HOLD for payment 2023-03-14] Implement better default avatars #12259

Closed

s77rt mentioned this pull request Mar 27, 2023

[HOLD for payment 2023-04-03] [$1000] [Android] Workspaces - The icon of a deleted offline workspace shows a circle instead of the standard shape #15697

Closed

6 tasks

arosiclair mentioned this pull request Apr 4, 2023

[HOLD for payment 2023-03-31] [$1000] Upload photo visible area is displayed as circle even though workspace image is square #15845

Closed

6 tasks

narefyev91 reviewed Oct 16, 2023

View reviewed changes

narefyev91 mentioned this pull request Oct 16, 2023

[HOLD for payment 2023-10-16] [$500] Web - image preview close on change image from other user #28074

Closed

6 tasks

eVoloshchak reviewed Nov 2, 2023

View reviewed changes

eVoloshchak mentioned this pull request Nov 2, 2023

[HOLD for payment 2023-11-02] [$125] Member list - Tooltip show phone number include "@expensify.sms" #29645

Closed

6 tasks

Uh oh!

Conversation

grgia commented Feb 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

shawnborton commented Feb 20, 2023

Uh oh!

shawnborton commented Feb 20, 2023

Uh oh!

MelvinBot commented Feb 21, 2023

Uh oh!

Uh oh!

mananjadhav Feb 22, 2023

Choose a reason for hiding this comment

Uh oh!

grgia Feb 24, 2023

Choose a reason for hiding this comment

Uh oh!

grgia Feb 24, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mananjadhav Feb 22, 2023

Choose a reason for hiding this comment

Uh oh!

grgia Feb 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mananjadhav left a comment

Choose a reason for hiding this comment

Uh oh!

aldo-expensify commented Mar 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer Checklist

Screenshots/Videos

Uh oh!

aldo-expensify commented Mar 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aldo-expensify commented Mar 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aldo-expensify left a comment

Choose a reason for hiding this comment

Uh oh!

grgia commented Mar 3, 2023

Uh oh!

mananjadhav commented Mar 3, 2023

Uh oh!

OSBotify commented Mar 3, 2023

Uh oh!

mananjadhav commented Mar 3, 2023

Uh oh!

mananjadhav commented Mar 3, 2023

Reviewer Checklist

Screenshots/Videos

Uh oh!

shawnborton commented Mar 3, 2023

Uh oh!

shawnborton commented Mar 3, 2023

Uh oh!

OSBotify commented Mar 6, 2023

Uh oh!

OSBotify commented Mar 7, 2023

Uh oh!

s77rt commented Mar 27, 2023

Uh oh!

arosiclair commented Apr 4, 2023

Uh oh!

narefyev91 Oct 16, 2023

Choose a reason for hiding this comment

grgia commented Feb 19, 2023 •

edited

Loading

grgia Feb 24, 2023 •

edited

Loading

aldo-expensify commented Mar 3, 2023 •

edited

Loading

aldo-expensify commented Mar 3, 2023 •

edited

Loading

aldo-expensify commented Mar 3, 2023 •

edited

Loading