Refactor avatar tooltips to remove shouldShowTooltip - #97961
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ 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". |
Guccio163
left a comment
There was a problem hiding this comment.
LGTM, I'd add some unit tests for the new components (AvatarTooltip, AvatarNamesTooltip, AvatarTooltipContext)
|
@eVoloshchak 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] |
|
cc. @roryabraham - side quest to remove shouldShowTooltip prop drill |
roryabraham
left a comment
There was a problem hiding this comment.
I was on the edge about whether these these comments should be blockers, but I guess they're not: this PR is definitely a nice improvement.
I'd recommend making the two changes I requested. Chatting with Claude about the risks of the useContext -> use change, I think it should be pretty safe to do.
| */ | ||
| function AvatarNamesTooltip({avatars, children}: AvatarNamesTooltipProps) { | ||
| const areTooltipsDisabled = useAreAvatarTooltipsDisabled(); | ||
| const {formatPhoneNumber, translate} = useLocalize(); |
There was a problem hiding this comment.
NAB, but an interesting note I think is worth considering. If we update useLocalize to use use instead of useContext under the hood, then it can be called conditionally, which would mean we can subscribe to the locale context only if areTooltipsDisabled is false.
There was a problem hiding this comment.
It is interesting, but not sure if worth. The subscription only cost is negligible and in the case useLocalize that rarely changes values I don't this we're bearing any re render costs here.
| }>; | ||
|
|
||
| /** Suppresses `AvatarTooltip` and `AvatarNamesTooltip` for every avatar rendered inside. */ | ||
| function AvatarTooltipsDisabled({isDisabled = true, children}: AvatarTooltipsDisabledProps) { |
There was a problem hiding this comment.
When I saw this, I was expecting to see a default provider very high in the tree (i.e: AuthScreens?) that sets isDisabled = false, preventing all avatar tooltips from becoming a no-op if not wrapped in the context lower in the tree.
There was a problem hiding this comment.
Could but it works the same w/o having a top lvl one. Will be good from clarity perspective.
I'll leave as is here - this PR I treat it as a step, not a final solution.
Ideally we'll have similar TooltipsDisabled that will come from reworked Tooltips the will replace AvatarTooltipsDisabled.
| }>; | ||
|
|
||
| /** Suppresses `AvatarTooltip` and `AvatarNamesTooltip` for every avatar rendered inside. */ | ||
| function AvatarTooltipsDisabled({isDisabled = true, children}: AvatarTooltipsDisabledProps) { |
There was a problem hiding this comment.
Also, I'm kind of wondering if this falls under the same umbrella as no-negated-variable-names lint rule. Maybe it should be AvatarTooltipsContextProvider, with isEnabled defaulting to true, and then to disable it you pass isEnabled={false}.
I think that's a bit more inline with the spirit of that custom lint rule.
There was a problem hiding this comment.
Given this and prev comment I think I'll rework it with global context providing default.
|
@eVoloshchak all yours |
Reviewer Checklist
Screenshots/VideosMacOS: Chrome / SafariScreen.Recording.2026-08-11.at.16.41.25.movScreen.Recording.2026-08-11.at.16.39.08.mov |
|
Looks and tests well! |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 roryabraham 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.4.53-0 🚀
|
No help site changes requiredI reviewed the changes in this PR and no updates to Expensify's help site files under Why: This is a pure internal code refactor. It extracts avatar tooltip logic out of the individual avatar layout components into reusable The help site documents user-facing features and workflows, not internal component architecture. Since there is no observable change to what users see or do, there is nothing to add or update in the docs. No draft PR was created — there's no linked help site PR to review. @jmusial, if you believe a user-facing behavior did change here that should be documented, let me know and I'll draft the help site update. |
Explanation of Change
Extracts avatar tooltip logic out of the individual avatar layout components into dedicated reusable
AvatarTooltip/AvatarNamesTooltipcomponents with anAvatarTooltipContextfor disabling tooltips, deduplicating tooltip wiring across layouts and their call sites.Fixed Issues
$ #94586
PROPOSAL:
Tests
Only on web
Test 1 — tooltips shown (unchanged behavior):
Test 2 — tooltips suppressed for archived rooms
Offline tests
Same as tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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
Videos just for web - mobile does not have tooltips anyways.
MacOS: Chrome / Safari
Screen.Recording.2026-08-10.at.11.18.43.mov