[Avatars] Split out Agent Avatars from User Avatars#92002
Conversation
|
🚧 @grgia has triggered a test Expensify/App build. You can view the workflow run here. |
melvin-bot
Bot
requested review from
JmillsExpensify and
brunovjk
and removed request for
a team
grgia
requested review from
a team and
NicolasBonet
and removed request for
JmillsExpensify and
brunovjk
|
@cretadn22 Please 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] |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3ec1a9968
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
This comment has been minimized.
This comment has been minimized.
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
dannymcclain
left a comment
dannymcclain
left a comment
There was a problem hiding this comment.
Looks good to me from a design perspective. No bots for users!
|
🚧 @grgia has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
Tests well to me 👍 |
grgia
changed the title
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb Chrome92002_android_web.moviOS: HybridApp
iOS: mWeb Safari92002_ios_web.movMacOS: Chrome / Safari
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -0,0 +1,26 @@ | |||
| /* eslint-disable @typescript-eslint/naming-convention */ | |||
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
The /* eslint-disable @typescript-eslint/naming-convention */ directive at the top of this new file lacks a justification comment explaining why the naming convention rule needs to be disabled. The keys in AGENT_AVATAR_ENTRIES use kebab-case (e.g., 'bot-avatar--blue') which conflicts with the naming convention rule, but this reason should be documented.
Add a justification comment explaining why the rule is disabled, for example:
/* eslint-disable @typescript-eslint/naming-convention -- Avatar IDs use kebab-case to match CDN filenames */Reviewed at: 95f3f70 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
NicolasBonet
left a comment
NicolasBonet
left a comment
There was a problem hiding this comment.
@grgia this PR seems way too big and complex for what we need, it's much easier to go with a filter over the avatars we show to the user:
// Bot avatars are reserved for agent accounts and must not be selectable by regular users.
---> const BOT_AVATAR_IDS = new Set<PresetAvatarID>(Object.keys(BOT_AVATARS) as BotAvatarIDs[]);
const buildOrderedAvatars = (): Array<{id: PresetAvatarID} & AvatarEntry> => {
---> const allIDS = (Object.keys(PRESET_AVATAR_CATALOG) as PresetAvatarID[]).filter((id) => !BOT_AVATAR_IDS.has(id));
const explicit = DISPLAY_ORDER.filter((id) => id in PRESET_AVATAR_CATALOG && !BOT_AVATAR_IDS.has(id));
const explicitSet = new Set<PresetAvatarID>(explicit);
const leftovers = allIDS.filter((id) => !explicitSet.has(id)).sort();
const finalIDOrder = [...explicit, ...leftovers];
return finalIDOrder.map((id) => ({
id,
...PRESET_AVATAR_CATALOG[id],
}));
};
That should be good enough unless I am missing something or an important flow?
|
@NicolasBonet i disagree, this PR is extracting logic. It's not complicated it's reusing the systems in place and makes it clearer for the next time. We're also going to add more workspace avatar customisation and this prepares for that |
Do you have issues or proposals for that avatar customization? That might be the bit I am not familiar with. |
5 participants
Explanation of Change
Should Appear Here:
Not Here:
Fixed Issues
$ #91980
PROPOSAL:
Tests
I tested by
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Verify that no errors appear in the JS console
Go to
/settings/profile/avatarand click to edit avatarVerify no Bot Avatars
Go to
/settings/agents/newclick to edit avatarVerify bot avatars appear
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)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