fix(Avatar): avatar size and loading skeleton #2880
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: 2ae8735 The changes in this PR will be included in the next version bump. This PR includes changesets to release 36 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
cf-remylenoir
force-pushed
the
fix-avatar-size-and-loading-skeleton
branch
from
6836819
to
c397fa0
Compare
cf-remylenoir
force-pushed
the
fix-avatar-size-and-loading-skeleton
branch
from
c397fa0
to
720a0cc
Compare
cf-remylenoir
force-pushed
the
fix-avatar-size-and-loading-skeleton
branch
from
720a0cc
to
0d375e0
Compare
| * @default 'medium' | ||
| */ | ||
| size?: Size; | ||
| size?: Size | string; |
There was a problem hiding this comment.
I would rather adhere to the sizes we have in our product than introduce this to break the rules 🤷
There was a problem hiding this comment.
Offering flexibility on top of our system isn't necessarily bad, especially for this component.
99% of the time, the Avatar will be used with a size variant to fit the overall composition and what designers followed in the Design System. With this change, the size is distributed up to 2 levels down (Avatar > Image + Skeleton loader), without CSS override.
That said, this is to satisfy the 1%, one could argue that having a CSS override with nested targeting to reach the image + skeleton is a good compromise too, and that we should not update the API.
@damann @andipaetzold thoughts?
There was a problem hiding this comment.
Without having a deep understanding of it, I’m not seeing the value in allowing overrides for images and skeletons. Could you provide an example of when these overrides would be necessary?
There was a problem hiding this comment.
The account setting page uses the Avatar component with a custom size via CSS override, which results in a broken experience (see first video in PR description).
With the proposed changes, the custom size can be specified via the Avatar props (e.g. 52px), does not require CSS override, and is propagated to the Image + skeleton loader components.
The flip side of the coin that Kristoffer mentions, is that with the proposed changes, we allow any custom value in (on top of the size variants system).
There was a problem hiding this comment.
I would rather adhere to the sizes we have in our product than introduce this to break the rules 🤷
I generally agree with that. Most other components don't allow that kind of override, but we limit users to specific values.
In this case, CSS overrides are not straight forward. The size impacts multiple CSS properties and relies on knowing the internal structure.
With the proposed type, we loose all autocompletion for the property. I would therefore go for Size | `${number}px` or similar. That way, the IDE still suggests the official sizes.
Alternatively, we could allow passing a custom size but hide that from the public API.
There was a problem hiding this comment.
Good point for the typing, template literal is a better approach.
cf-remylenoir
force-pushed
the
fix-avatar-size-and-loading-skeleton
branch
from
a118eda
to
2ae8735
Compare
Purpose of PR
Fixes the Avatar's loading skeleton rendering.
We extend the size property to receive custom sizes. This should prevent people from using custom styling that does not deeply apply to the Avatar > Image > Skeleton loader component, see below:
Current rendering with custom styling
size-with-custom-styling.mov
Loading skeleton
Tip
One can observe the loading skeleton before/after in the deployed Storybook by blocking the request to the avatar image:
Before
before.mov
After
after.mov
Storybook update
LoadingsectionScreen.Recording.2024-09-18.at.14.30.46.mov
PR Checklist
readme.mdfile(s)