[No QA] Support Dual-Initial Avatars in App - #94567
Conversation
|
@codex review |
![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: 39a1bef4fc
ℹ️ 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".
…dd cspell word
- Avatar now calls usePermissions(), which iterates Permissions keys and
invokes each as a checker function. The Permissions test mock spread the
module namespace ({default, isBetaEnabled}) instead of the default export,
so the non-function 'default' key crashed usePermissions in LHN/Sidebar
test render paths. Spread .default so only function checkers are exposed.
- Add 'uppercased' to cspell wordlist (used in a UserAvatarUtils comment).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 423c7267d4
ℹ️ 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".
- Move UserInitialsAvatar static styles into StyleSheet and name the font-size ratio - Note why the local default reads initials from the email, not the name - Add a regression test that a generated letter URL is not a default avatar
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea0fdb1672
ℹ️ 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".
grgia
changed the title
|
@thelullabyy the BE PRs will actually send the letters, but the FE PR will be merged first |
|
🚧 grgia has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
…ion, fix typo - Remove the unused CONST.BETAS.DEFAULT_LETTER_AVATARS flag (never referenced; the feature ships without a beta gate). - Inline the letter-avatar render check to a single condition instead of a redundant shouldRenderLetterAvatar boolean plus a repeated truthiness test. - Fix 'Weather' -> 'Whether' typo in AvatarPreview.
|
Almost, but not a strict no-op and I want to be upfront about it. Existing accounts get their avatar from the BE, which isn't emitting letter URLs yet, so they're unchanged until Auth + Web deploy. The exception is client-side defaults: |
|
@thelullabyy added claude responses to help explain the thought process without the other PRs |
|
@NicolasBonet 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] |
|
@NikkiWines im sure you have loads on your plate, but incase you feel like taking review on these 3 avatar PRs, given you have some context- lemme know! app - #94567 |
Reviewer Checklist
|
| for (const character of name) { | ||
| // A non-ASCII codepoint has no pre-generated image, so fall back instead of skipping to a | ||
| // later ASCII letter, which would be a misleading initial for the name. | ||
| if ((character.codePointAt(0) ?? 0) >= 0x80) { | ||
| return ''; | ||
| } | ||
| if (/[a-z0-9]/i.test(character)) { | ||
| return character.toUpperCase(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Should we consider cases like Élodie Lee -> EL? I believe that we this we will only show L for that case and we might want to fallback some characters.
There was a problem hiding this comment.
Ooo great point, let me check what those characters entail
|
@grgia feel free to merge if all workflows are green! |
|
🚧 NicolasBonet has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/NicolasBonet in version: 9.4.27-1 🚀
|
📝 Help site review — changes requiredYes. This PR (with its Auth/Web counterparts) replaces the auto-assigned illustrated/"cartoon-style" default avatar with an initials-based letter avatar for photo-less users. Two existing help articles describe the old default-avatar behavior and are now inaccurate, so I've drafted the updates. Draft PR: #95296 What changed:
The wording follows the client logic in @grgia, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
Deploy Blocker #95360 was identified to be related to this PR. |
|
🚀 Deployed to production by https://github.com/grgia in version: 9.4.27-2 🚀
Bundle Size Analysis (Sentry): |
| @@ -102,12 +108,8 @@ function AvatarPreview({selected, avatarCaptureRef, setSelected, isAvatarCropMod | |||
| }; | |||
|
|
|||
| const onImageRemoved = () => { | |||
There was a problem hiding this comment.
Doing this directly without saving the option led to #95360
7 participants
Explanation of Change
NO QA:
Confirmed on adhoc that avatars are the same. We will QA after the BE issues are merged. The App PR is safe for merge.
Deploy strategy — no beta gate
The backend emits letter-avatar URLs unconditionally for photo-less users, so the deploy is the launch.
To test- check out
Auth - https://github.com/Expensify/Auth/pull/22494
Web - https://github.com/Expensify/Web-Expensify/pull/54007
Confirmed that other avatars not impacted:
Fixed Issues
$ #93784
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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.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