feat(svelte-ds-app-launchpad): Add UserAvatar component#606
feat(svelte-ds-app-launchpad): Add UserAvatar component#606
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new UserAvatar component to the Svelte Launchpad design system, providing an avatar image with fallbacks to user initials or a generic icon, along with Storybook coverage and tests.
Changes:
- Introduce
UserAvatarSvelte component with size variants and image/initials/icon fallback logic. - Add
getInitialsutility with unit tests. - Add Storybook stories plus browser + SSR tests, and export the component from the components barrel.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/svelte/ds-app-launchpad/src/lib/components/UserAvatar/UserAvatar.svelte | Implements the new UserAvatar component rendering logic and fallbacks |
| packages/svelte/ds-app-launchpad/src/lib/components/UserAvatar/types.ts | Defines public props/types for UserAvatar |
| packages/svelte/ds-app-launchpad/src/lib/components/UserAvatar/styles.css | Adds styling for avatar, sizes, and fallback presentation |
| packages/svelte/ds-app-launchpad/src/lib/components/UserAvatar/index.ts | Exports UserAvatar component and types |
| packages/svelte/ds-app-launchpad/src/lib/components/UserAvatar/UserAvatar.stories.svelte | Adds Storybook stories for typical and edge-case states |
| packages/svelte/ds-app-launchpad/src/lib/components/UserAvatar/UserAvatar.svelte.test.ts | Adds browser-level component tests |
| packages/svelte/ds-app-launchpad/src/lib/components/UserAvatar/UserAvatar.ssr.test.ts | Adds SSR rendering tests |
| packages/svelte/ds-app-launchpad/src/lib/components/UserAvatar/utils/getInitials.ts | Implements initials extraction helper |
| packages/svelte/ds-app-launchpad/src/lib/components/UserAvatar/utils/getInitials.test.ts | Adds unit tests for initials extraction helper |
| packages/svelte/ds-app-launchpad/src/lib/components/UserAvatar/utils/index.ts | Re-exports getInitials utility |
| packages/svelte/ds-app-launchpad/src/lib/components/index.ts | Re-exports UserAvatar from the main components barrel |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
d538d60 to
8264b48
Compare
| }; | ||
|
|
||
| export interface UserAvatarProps | ||
| extends Omit<HTMLAttributes<HTMLElement>, "children">, |
There was a problem hiding this comment.
Would we like callers to be able to pass image props here? In that case, it would be nice to extend HTMLAttributes<HTMLImageElement>, but this is complicated because the root element of this component is a div in some cases.
Maybe something like this would work (haven't tried this):
interface BaseAvatarProps {
size?: "small" | "medium" | "large";
class?: string;
}
interface AvatarWithImage extends BaseAvatarProps, Omit<HTMLImgAttributes, "size" | "src"> {
userAvatarUrl: string;
userName?: string;
}
interface AvatarWithoutImage extends BaseAvatarProps, Omit<HTMLAttributes<HTMLDivElement>, "size"> {
userAvatarUrl?: never; // This is the magic "discriminated" part
userName?: string;
}
export type UserAvatarProps = AvatarWithImage | AvatarWithoutImage;There was a problem hiding this comment.
This approach would be problematic here, because the root element type not only depends on the presence of userAvatarUrl (where this would be ok to use), but also on the possibility of an image loading error when the img is replaced with the div.
What we could do is to manually pick a subset of the most commonly used img attributes, destructure them from rest in $props() definition and only forward to the img element. If you think that's the move, what attributes should we choose? alt, decoding, fetchpriority, loading? Do you have any other ideas on how to handle that?
There was a problem hiding this comment.
My gut is to restructure things somewhat so that we actually have different components based on what the root element is to simplify the types, so we don't need to own the maintenance of picking "commonly used" attributes.
UserAvatarthat controls the logic for determining which specific component to use based on loading state, etcUserAvatarImagethat is always an imageUserAvatarPlaceholderor other name that is the div used in error/loading
Then at the UserAvatar level you could expose two sets of props, imageProps, and placeholderProps, and spread those onto the respective elements when they are rendered. Do you think this is any nicer?
There was a problem hiding this comment.
My main concern is that we'd be trading our
maintenance of picking "commonly used" attributes
for a more complex consumer API - one that doesn't occur in any of our other components so far.
For example, if a consumer wants to apply a class and an aria-label, they would be forced to specify those props twice:
<UserAvatar
userName="John"
imageAttributes={{
src: "https://assets.ubuntu.com/v1/fca94c45-snap+icon.png",
alt: "User Avatar",
class: "my-class",
"aria-label": "User Avatar",
}}
placeholderAttributes={{
class: "my-class",
"aria-label": "User Avatar",
}}
/>If we were to eventually separate things further into imageProps, initialsProps, and iconProps, they might even have to define them three times! And to be frank, I am not a fan of this direction.
It also forces us into other tricky decisions. For example - regarding attachments. Should each root element have its own way of passing an attachment to keep things consistent but force the user into questionable ergonomics of relying on createAttachmentKey?
<UserAvatar
imageAttributes={{
[createAttachmentKey()]: (el) => {
// ...
},
}}
placeholderAttributes={{
[createAttachmentKey()]: (el) => {
// ...
},
}}
/>Or, do we expose one shared attachment at the root level, breaking the pattern of grouping things by element?
<UserAvatar
imageAttributes={{
//...
}}
placeholderAttributes={{
//...
}}
{@attach (el) => {
// ...
}}
/>Personally, I value keeping the consumer API simple, ergonomic, and consistent with the rest of our components, even if it means we have to take on a bit of extra maintenance under the hood.
If our main goal is to avoid making a decision on "which img-specific attributes to choose", I would prefer to just manually expose all that the standard currently defines:
altcrossorigindecodingfetchpriorityheightismaploadingreferrerpolicysizessrcsrcsetusemapwidth
There really aren't that many of them, and I wouldn't expect the list of standard img attributes to change very often anyway.
Let me know what you think.
There was a problem hiding this comment.
I came up with another, somewhat "hybrid" option that I think is the best middle ground between the two approaches. The idea is to keep all attributes that are shared between the root elements as flat component props and allow for imageAttributes that'd contain only the ones that are img-specific.
import type { HTMLAttributes, HTMLImgAttributes } from "svelte/elements";
type ImageOnlyAttributes = Omit<
HTMLImgAttributes,
| keyof HTMLAttributes<HTMLElement>
| "bind:naturalWidth"
| "bind:naturalHeight"
| "children"
>;
export interface UserAvatarProps extends Omit<
HTMLAttributes<HTMLElement>,
"children"
> {
userName?: string;
size?: "small" | "medium" | "large";
imageAttributes?: ImageOnlyAttributes;
}And then:
<script lang="ts">
const {
class: classProp,
userName: userNameProp,
size = "medium",
imageAttributes,
...rest
}: UserAvatarProps = $props();
// ...
</script>
{#if imageAttributes?.src && !imageError}
<img
class={className}
title={userName || undefined}
data-initials={userInitials}
onerror={() => (imageError = true)}
{...imageAttributes}
{...rest}
/>
{:else if userName}
<abbr class={className} title={userName} {...rest}>
{userInitials}
</abbr>
{:else}
<div class={className} {...rest}>
<UserIcon />
</div>
{/if}This raises a different question, related to your other comment. If we move forward with this approach, do we:
- Keep
srcwithinimageAttributes. This splits user-related data across different levels (theuserNameroot property vs.imageAttributes.src). - Remove
srcfromimageAttributesand move it to the root asuserImageUrl, placing it directly alongsideuserName. - Remove
srcfromimageAttributesand create a root-leveluserobject containing bothnameandurl.
I personally lean toward Option 2, as it surfaces the most important properties for better visibility.
Please let me know what your thoughts are.
| /** The URL of the user's avatar image */ | ||
| userAvatarUrl?: string; |
There was a problem hiding this comment.
Why can't we just use the native src for this?
There was a problem hiding this comment.
We decided to use userAvatarUrl to keep the UserOptions type reusable and decoupled from the specifics of the img element (with all its properties relating to a user "user..."). You can see an example of its use in the Timeline.Event component.
We could stick to src here and e.g., map userAvatarUrl to it within the Timeline, but that would create an API inconsistency.
Please let me know which approach you prefer :)
There was a problem hiding this comment.
We could stick to src here and e.g., map userAvatarUrl to it within the Timeline, but that would create an API inconsistency.
Is that really an "inconsistency", or is it a more honest modeling of how the information's meaning is different from component to component? At the level of the Timeline, it makes sense to scope these props to "user" because it's a different information space than the component in question. In the user avatar, though, it feels a bit redundant to repeat user... in these props (the component name already tells us this is related to a user).
We should be sure this is a sound approach though, I don't want to make the API or implementation cumbersome due to this. Do you have any opinions here @nathanclairmonte @jademathre-canonical @goulinkh @alvaromateo ?
There was a problem hiding this comment.
I like this framing, and I'm happy with either option. Let's see what other folks think!
| /** The URL of the user's avatar image */ | ||
| userAvatarUrl?: string; |
There was a problem hiding this comment.
We could stick to src here and e.g., map userAvatarUrl to it within the Timeline, but that would create an API inconsistency.
Is that really an "inconsistency", or is it a more honest modeling of how the information's meaning is different from component to component? At the level of the Timeline, it makes sense to scope these props to "user" because it's a different information space than the component in question. In the user avatar, though, it feels a bit redundant to repeat user... in these props (the component name already tells us this is related to a user).
We should be sure this is a sound approach though, I don't want to make the API or implementation cumbersome due to this. Do you have any opinions here @nathanclairmonte @jademathre-canonical @goulinkh @alvaromateo ?
| }; | ||
|
|
||
| export interface UserAvatarProps | ||
| extends Omit<HTMLAttributes<HTMLElement>, "children">, |
There was a problem hiding this comment.
My gut is to restructure things somewhat so that we actually have different components based on what the root element is to simplify the types, so we don't need to own the maintenance of picking "commonly used" attributes.
UserAvatarthat controls the logic for determining which specific component to use based on loading state, etcUserAvatarImagethat is always an imageUserAvatarPlaceholderor other name that is the div used in error/loading
Then at the UserAvatar level you could expose two sets of props, imageProps, and placeholderProps, and spread those onto the respective elements when they are rendered. Do you think this is any nicer?
Done
UserAvatarcomponent that displays an avatar image when available, falls back to user initials ifuserNameis provided, and a generic icon placeholder otherwise.::afteron the<img>ifuserNameis provided and a?otherwise.QA
bun run check && bun run testComponents/UserAvatarin StorybookuserAvatarUrlis providedPR readiness check
Feature 🎁,Breaking Change 💣,Bug 🐛,Documentation 📝,Maintenance 🔨.package.json:check,check:fix, andtest.buildto build the package for development or distribution,build:allto build all artifacts. See CONTRIBUTING.md for details.npm publish --access public(first-time publishing is not automated). Runbun run publish:statusfrom the repo root to verify.Screenshots