-
Notifications
You must be signed in to change notification settings - Fork 0
Review 4327
#4327 — feat(Avatar): add shape prop (circle | rounded | square) for non-circular form factors (#4205)
HelloOjasMutreja · open · view on GitHub
Verdict: request-changes · 2 rounds
These records predate the Review Presentation format for the most part, so the problem/solution/API/theme-target/breaking slots are only filled where the review itself carried them. Reviews from 2026-08-23 on use the full template.
Thanks for this — the fallback chain and the status-dot placement are the fiddly parts and you got them right.
The one I can't take as-is: shape only reaches an instance. The clipping box is the inner div, which no theme target touches, so
.astryx-avatar {border-radius: 8px}leaves the avatar a circle — while the docs here promise a theme can set it globally. In a group of squares the "+N" chip also stays a pill, and a consumer can't reach it to fix it.Both fall out if the radius goes through one component token, the way chat surfaces have
--radius-chat:circle: {'--radius-avatar': radiusVars['--radius-full']}, rounded: {'--radius-avatar': radiusVars['--radius-element']}, square: {'--radius-avatar': radiusVars['--radius-none']},with content, wrapper and the overflow chip all reading
--radius-avatar.Does that cover the rounded-square case you were after?
[Reviewed by Robohands]
Thanks for turning this so fast, and
--_avatar-radiusis the right name — it matches--_card-radiusand the private-var convention exactly.What's missing is the registration that convention needs. A theme writing
avatar: {base: {borderRadius: '10px'}}gets onlyborder-radiuson the wrapper, so the photo still clips to a circle and shape stays per-instance. Card works because of:vars: [{name: '--_avatar-radius', private: true, /* … */}], derived: [{property: 'borderRadius', vars: ['--_avatar-radius']}],plus the
derivedVarRegistryentry — which is what the red test is asking for. Setting the var directly already changes every surface, so the plumbing is done.Separately,
shapeStylesis now a public export of core. Could AvatarGroupOverflow import it from./Avatar/Avatarso it stays internal?[Reviewed by Robohands]