Migrate Avatar to new Compound component #11393
Conversation
germain-gg
requested review from
t3chguy,
SimonBrandner,
daniellekirkwood and
callumu
germain-gg
changed the title
germain-gg
added
the
T-Enhancement
| /> | ||
| ); | ||
| } | ||
| return ( |
There was a problem hiding this comment.
Thanks, not sure why ESLint was not shouting at me, back in use now.
There was a problem hiding this comment.
The rule is disabled on this block
There was a problem hiding this comment.
Unfortunately https://github.com/matrix-org/matrix-react-sdk/pull/11393/files#diff-f61169b723cd2eea456ca1c6fa8879aaf150f49060d77d4d8bf42257093e078fR114 applies to the whole block not just that one line
| @@ -82,7 +82,7 @@ export default class RoomAvatarEvent extends React.Component<IProps> { | |||
| className="mx_RoomAvatarEvent_avatar" | |||
| onClick={this.onAvatarClick} | |||
| > | |||
| <RoomAvatar width={14} height={14} oobData={oobData} /> | |||
| <RoomAvatar width={16} oobData={oobData} /> | |||
There was a problem hiding this comment.
Can this happen in a different PR?
There was a problem hiding this comment.
Not quite, the new avatar uses box-sizing: border-box, so if i do not do that change the avatars end up being two pixels smaller.
There was a problem hiding this comment.
But it looks like it genuinely wants a 14px (OD) avatar?
| altText = _t("Avatar"), | ||
| ariaLabel = _t("Avatar"), |
There was a problem hiding this comment.
Losing this seems bad, this is the aria-label for the button which wraps clickable avatars
t3chguy
added
the
X-Needs-Percy
|
Any chance of screenshots of more variants, with globe cutout, with online indicator cutout, user info (large), space home, etc etc |
| aria-label={ariaLabel} | ||
| aria-live="off" |
| ); | ||
| const imgNode = ( | ||
| <img | ||
| loading="lazy" |
There was a problem hiding this comment.
Added to element-hq/compound-web#57
There was a problem hiding this comment.
Added to element-hq/compound-web#57
| aria-hidden="true" | ||
| data-testid="avatar-img" |
| className={classNames("mx_BaseAvatar", className)} | ||
| ref={inputRef} | ||
| {...otherProps} | ||
| role="presentation" |
| /** | ||
| * @deprecated use `width` only, avatars have a 1:1 aspect ratio | ||
| */ | ||
| height: number; |
There was a problem hiding this comment.
Can we just kill it? We're not a real SDK so don't need backwards compatibility, confusion is worse than bigger diffs
There was a problem hiding this comment.
Done! I will rename width to size in a follow up PR
|
Supplemented by #11448. |
Uses https://compound.element.io/?path=/story/compound-web_avatar--round
Checklist
Here's what your changelog entry will look like:
✨ Features