fix: Restore previous Avatar implementation as DeprecatedAvatar#1058
Open
pawelgrimm wants to merge 4 commits into
Open
fix: Restore previous Avatar implementation as DeprecatedAvatar#1058pawelgrimm wants to merge 4 commits into
pawelgrimm wants to merge 4 commits into
Conversation
…sts + snapshot - un-inline utils into utils.ts; restore utils.test.ts (9 cases) - restore full component tests + snapshot from src/avatar@65811a36 - revert churn so every file is pure rename of original - keep @deprecated JSDoc, displayName, type exports Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
84c1f94 to
69f517e
Compare
doistbot
reviewed
Jun 4, 2026
Member
doistbot
left a comment
There was a problem hiding this comment.
Thanks Paweł for safely restoring the legacy Avatar component to ease the migration path 😄 🙌.
Few things worth tightening:
- API conventions: Wrap the component in
React.forwardRefand extend standard HTMLdivprops (omittingstyle) so consumers can attach refs and standard attributes. - Accessibility: Provide semantic markup for the image by either using an actual
<img>tag or addingrole="img"with a derivedaria-label. - Performance: Swap the root
Boxcomponent for a plaindivto avoid unnecessary responsive resolution overhead. - Test coverage: Add a test exercising the restored
colorListprop to ensure the legacy behavior is fully protected.
I also included a few optional follow-up notes in the details below.
Optional follow-up notes (4)
- [P3] src/deprecated-avatar/deprecated-avatar.test.tsx:7: The test suite is missing an accessibility test (
jest-axe). Project guidelines mandate that every component should have an a11y test, even if this is a restored deprecated component. - [P3] src/deprecated-avatar/deprecated-avatar.stories.tsx:103: Spreading
...argsdirectly passes the story'suserNameandemailkeys as props toDeprecatedAvatar, which spreads them ontoBoxand ultimately the DOMdiv. This will trigger a React "does not recognize prop on a DOM element" warning in the console. Consider destructuring these properties out before spreading the rest of the arguments. - [P3] src/deprecated-avatar/deprecated-avatar.tsx:67:
colorListis currently typed as anystring[], socolorList={[]}is a valid call. In that caseemailToIndex(user.email, colorList.length)becomes... % 0, which returnsNaNand leavesbackgroundColorunset, so the avatar can render white initials on no background. Guard the empty-array case here, or fall back toAVATAR_COLORSwhencolorList.length === 0. - [P3] src/deprecated-avatar/deprecated-avatar.test.tsx:13:
toMatchSnapshot()is locking the entire rendered node here, including Box's class composition, for a behavior that's really just “usesavatarUrl”. A focused assertion onbackgroundImage/textIndentwould cover the contract with less churn when unrelated markup or class details change.
|
|
||
| import { DeprecatedAvatar } from './deprecated-avatar' | ||
|
|
||
| describe('DeprecatedAvatar', () => { |
Member
There was a problem hiding this comment.
🟡 P2 This suite never exercises the restored colorList prop, even though preserving that legacy API is one of the goals of this PR. Please add a case that passes a custom list and asserts the resulting backgroundColor, otherwise a regression here will slip through while the initials-only tests still pass.
3 tasks
Contributor
Author
|
Doistbot made some good points, but we're not reworking the avatar here, we're just bringing back the previous implementation under the "Deprecated" namespace. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Short description
Restores the previous Avatar implementation as
DeprecatedAvatarwhile keeping the revampedAvatarexport in place.The deprecated component keeps legacy props such as
user,avatarUrl,colorList, and responsive string sizes with scoped CSS module styles, and now lives insrc/deprecated-avatar.Restores the legacy Avatar stories under
Deprecated Components/Avatarand moves the deprecated Input story into the same section.PR Checklist