feat!: Revamped Avatar component#1048
Conversation
a953da4 to
8f6ff49
Compare
doistbot
left a comment
There was a problem hiding this comment.
Thanks pawelgrimm for your contribution 😊. The new fallback handling and dynamic image selection look solid, and the updated documentation featuring contributor avatars is a great touch.
Few things worth tightening:
- Wrap the component in
React.forwardRefand restore the...propsrest spread to the root element to maintain a flexible, standard API. - Derive the default fallback label from the normalized
nameinstead of the raw prop so whitespace-only inputs don't yield a blank accessible name. - Add a base
jest-axetest to the suite per our guidelines, alongside a targeted test verifying that customalttext is correctly exposed on the initials fallback.
I also included a few optional follow-up notes in the details below.
Optional follow-up notes (6)
- [P3] src/avatar/avatar.stories.tsx:9: This duplicates the supported-size list that already exists as
AVATAR_SIZESinutils.ts. If the component contract changes, the stories/playground can drift from the actual runtime/type surface. Import the shared constant here instead of maintaining a second copy. - [P3] src/avatar/utils.ts:74:
normalizeAvatarNamealready replaces all whitespace sequences with a single space. Using.split(' ')here produces identical results and avoids running the Unicode regex engine a second time. - [P3] src/avatar/utils.ts:147: Consider adding a fast-path
if (failedSources.length === 0) return imageProps;before checkingimageProps.sources. This avoids re-filtering the array and reconstructing the image properties object on every render for the typical happy path where no image sources have failed. - [P3] src/avatar/avatar.tsx:102:
getInitials(name)andgetAvatarMetaColorIndex(name)now run on every render, even whenavailableImageSourcesis truthy and the component never uses the initials/meta-color path. For image-heavy avatar lists, that adds avoidable normalization/segmentation/hash work to the hot path. Computing these only when we actually fall back to initials would avoid that per-render overhead. - [P3] src/avatar/avatar.mdx:25: This docs page now renders all of the image-heavy canvases inline, and most of those stories use live GitHub avatar URLs. Opening
Avatar.mdxwill trigger roughly 26 external image requests up front, which makes Storybook/Chromatic slower and less deterministic than it needs to be. Using local fixtures (or trimming the number of inline canvases) would avoid that resource overhead. - [P3] src/avatar/avatar.tsx:113: This new
metaColor-*class family is camel-cased, but Reactist’s CSS naming convention is dash-separated (variant-primary,size-small, etc.). Please rename these lookups/classes to something likemeta-color-*so Avatar stays consistent with the rest of the library.
rmartins90
left a comment
There was a problem hiding this comment.
Left 3 minor comments @pawelgrimm. Other than that, it looks good to me
f86eb30 to
30fca01
Compare
|
🎉 This PR is included in version 32.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
@pawelgrimm Don't the changes in this now mean the Avatar component is no longer responsive? What if I'm looking at the web app on mobile? I'm now getting the same avatar size as on desktop, right? v32 drops the responsive |
@scottlovegrove Yes 😅
I dropped the responsive sizing intentionally since 1) we weren't using it internally anywhere, and 2) it would have made the I'm not at all opposed to you adding it back in, but if I were you, I'd just duplicate the avatar behind media-query driven containers OR dynamically select a size in JS based on the screen size. |
sigh 🙄
How does this not go against the principles of this library? It should be responsive, everything was built for responsiveness, it's a web app UI package! |
I'm also a bit concerned about removing functionality in this way. Does this mean then that we're going to remove the |
I’m sorry this blocked you from upgrading, @scottlovegrove. I prepared #1058 for you, which restores the previous Avatar implementation as A robust component library should bias towards responsiveness, I won't disagree with you there, but I genuinely thought I was acting in line with the existing conventions in this library. Reactist supports responsive behavior today in that we have a responsive prop system, but it is applied selectively. From what I can see, responsive props are used exclusively in the layout primitives ( Responsive sizing on Avatar seemed more similar to the latter set of component, so I dropped it given the new implementation complexity around numeric sizes and
I think you raise a good point, @gnapse! Should we add and maintain features in Reactist that we don’t use internally? If yes, why and what criteria should we use for deciding that? How should we handle deprecating functionality? Given the renewed interest in this library, I think it's a good time to codify our stance on these kinds of principles. It'll be much easier to make calls in a consistent and predictable manner. @frankieyan Is this something you'd be interested in taking point on as part of your component modernization work? |
|
Yeah, I can bring this up with the designers and see where responsiveness stands for us. I would prefer that we roll the responsive prop into the new Avatar if it doesn't overly complicate it, over having to keep both copies around for long, as I've just started to remove some of our older deprecated components. @pawelgrimm would this make the follow-up avatar groups difficult to implement? The last thing I'd want to do is to delay your timelines. Baking responsiveness into our products would make better experiences. It's never something we spent alot of effort on beyond making it serviceable, and comparing Todoist's mobile app vs mobile web, our (web) avatars, which aren't even using Reactist components, are definitely undersized. That said, I'd like to get product and design's input first and ensure our apps can/will make use of them before we invest our time here. Ultimately, I think we should cater to our products' requirements first and include a similar callout as Typist:
And if we end up deciding mobile web experiences are not something we want to improve in the near term, I'll make that explicit both here in Reactist and in the handbook. |
@frankieyan That's exactly what I'm afraid of. I'll try reworking the API to make responsive sizing fit in, but then I might have to abandon the approach I'm taking to clip avatars and just require consumers to pass in the background color. I'm talking about the space between each avatar in the group, as you see here: One approach is to use a background-colored border around each avatar. The other is to clip the avatars so that the background content is shown through the gap. I thought the latter had much better ergonomics... at the cost of massively complicated CSS 🙈
Oh don't worry, I've already de-prioritized those components for now 😅 I was planning on picking them back up when I have some more slack. |


Summary
We are bringing the Avatar component up to spec with the design, including:
srcset)We also added a new documentation page for the component, which walks through all the features it supports. The new and improved stories feature all the contributors to this repo from the past year 😁
We also removed some features that we don't really need:
mediaquery-based responsive sizingname, if desired)Sorry for the rather large PR — I'd split it up, but that would mean pushing accompanying stories, docs, and tests in a separate PR, which isn't great either. Lmk if you feel strongly about having a smaller PR in this case and I can do my best to rework it.
References
📸 Demo
PR Checklist