Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Migrate Avatar to new Compound component #11393

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@
"@sentry/tracing": "^7.0.0",
"@testing-library/react-hooks": "^8.0.1",
"@vector-im/compound-design-tokens": "^0.0.3",
"@vector-im/compound-web": "^0.2.3",
"@vector-im/compound-web": "^0.2.4",
"await-lock": "^2.1.0",
"blurhash": "^1.1.3",
"classnames": "^2.2.6",
Expand Down
49 changes: 1 addition & 48 deletions res/css/views/avatars/_BaseAvatar.pcss
Original file line number Diff line number Diff line change
Expand Up @@ -14,57 +14,10 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

.mx_BaseAvatar {
position: relative;
/* In at least Firefox, the case of relative positioned inline elements */
/* (such as mx_BaseAvatar) with absolute positioned children (such as */
/* mx_BaseAvatar_initial) is a dark corner full of spider webs. It will give */
/* different results during full reflow of the page vs. incremental reflow */
/* of small portions. While that's surely a browser bug, we can avoid it by */
/* using `inline-block` instead of the default `inline`. */
/* https://github.com/vector-im/element-web/issues/5594 */
/* https://bugzilla.mozilla.org/show_bug.cgi?id=1535053 */
/* https://bugzilla.mozilla.org/show_bug.cgi?id=255139 */
display: inline-block;
user-select: none;

&.mx_RoomAvatar_isSpaceRoom {
&.mx_BaseAvatar_image,
.mx_BaseAvatar_image {
border-radius: 8px;
}
}
}

.mx_BaseAvatar_initial {
position: absolute;
left: 0;
color: $avatar-initial-color;
text-align: center;
speak: none;
pointer-events: none;
font-weight: normal;
}

.mx_BaseAvatar_image {
object-fit: cover;
aspect-ratio: 1;
border-radius: 125px;
vertical-align: top;
background-color: $background;
}

/* Percy screenshot test specific CSS */
@media only percy {
/* Stick the default room avatar colour, so it doesn't cause a false diff on the screenshot */
.mx_BaseAvatar_initial {
.mx_BaseAvatar {
background-color: var(--percy-color-avatar) !important;
border-radius: 125px;
}
.mx_RoomAvatar_isSpaceRoom .mx_BaseAvatar_initial {
border-radius: 8px;
}
.mx_BaseAvatar_initial + .mx_BaseAvatar_image {
visibility: hidden;
}
}
6 changes: 3 additions & 3 deletions res/css/views/avatars/_DecoratedRoomAvatar.pcss
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@ limitations under the License.
.mx_DecoratedRoomAvatar_icon {
position: absolute;
/* the following percentage based sizings are to match the scalable svg mask for the cutout */
bottom: -6.25%;
right: -6.25%;
margin: 12.5%;
bottom: 0;
right: 0;
transform: translate(-25%, -70%);
width: 25%;
height: 25%;
border-radius: 50%;
Expand Down
140 changes: 28 additions & 112 deletions src/components/views/avatars/BaseAvatar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,33 +20,36 @@ limitations under the License.
import React, { useCallback, useContext, useEffect, useState } from "react";
import classNames from "classnames";
import { ResizeMethod, ClientEvent } from "matrix-js-sdk/src/matrix";
import { Avatar } from "@vector-im/compound-web";

import * as AvatarLogic from "../../../Avatar";
import SettingsStore from "../../../settings/SettingsStore";
import AccessibleButton, { ButtonEvent } from "../elements/AccessibleButton";
import { ButtonEvent } from "../elements/AccessibleButton";
import RoomContext from "../../../contexts/RoomContext";
import MatrixClientContext from "../../../contexts/MatrixClientContext";
import { useTypedEventEmitter } from "../../../hooks/useEventEmitter";
import { toPx } from "../../../utils/units";
import { _t } from "../../../languageHandler";

interface IProps {
name?: string; // The name (first initial used as default)
idName?: string; // ID for generating hash colours
name?: React.ComponentProps<typeof Avatar>["name"]; // The name (first initial used as default)
idName?: React.ComponentProps<typeof Avatar>["id"]; // ID for generating hash colours
title?: string; // onHover title text
url?: string | null; // highest priority of them all, shortcut to set in urls[0]
urls?: string[]; // [highest_priority, ... , lowest_priority]
type?: React.ComponentProps<typeof Avatar>["type"];
width: number;
/**
* @deprecated use `width` only, avatars have a 1:1 aspect ratio
*/
height: number;
Comment on lines +40 to 43
Copy link
Member

@t3chguy t3chguy Aug 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just kill it? We're not a real SDK so don't need backwards compatibility, confusion is worse than bigger diffs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! I will rename width to size in a follow up PR

// XXX: resizeMethod not actually used.
/**
* @deprecated only uses `crop`
*/
resizeMethod?: ResizeMethod;
defaultToInitialLetter?: boolean; // true to add default url
onClick?: (ev: ButtonEvent) => void;
inputRef?: React.RefObject<HTMLImageElement & HTMLSpanElement>;
inputRef?: React.RefObject<HTMLSpanElement>;
className?: string;
tabIndex?: number;
altText?: string;
ariaLabel?: string;
}

const calculateUrls = (url?: string | null, urls?: string[], lowBandwidth = false): string[] => {
Expand Down Expand Up @@ -108,119 +111,32 @@ const BaseAvatar: React.FC<IProps> = (props) => {
url,
urls,
width = 40,
height = 40,
resizeMethod = "crop", // eslint-disable-line @typescript-eslint/no-unused-vars
defaultToInitialLetter = true,
onClick,
inputRef,
className,
type = "round",
altText = _t("Avatar"),
ariaLabel = _t("Avatar"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Losing this seems bad, this is the aria-label for the button which wraps clickable avatars

...otherProps
} = props;

const [imageUrl, onError] = useImageUrl({ url, urls });

if (!imageUrl && defaultToInitialLetter && name) {
const initialLetter = AvatarLogic.getInitialLetter(name);
const textNode = (
<span
className="mx_BaseAvatar_initial"
aria-hidden="true"
style={{
fontSize: toPx(width * 0.65),
width: toPx(width),
lineHeight: toPx(height),
}}
>
{initialLetter}
</span>
);
const imgNode = (
<img
loading="lazy"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Losing this seems bad

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

className="mx_BaseAvatar_image"
src={AvatarLogic.defaultAvatarUrlForString(idName || name)}
alt=""
title={title}
onError={onError}
style={{
width: toPx(width),
height: toPx(height),
}}
aria-hidden="true"
data-testid="avatar-img"
Comment on lines -151 to -152
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Losing these seems bad

/>
);

if (onClick) {
return (
<AccessibleButton
aria-label={ariaLabel}
aria-live="off"
Comment on lines -159 to -160
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Losing these seems bad

{...otherProps}
element="span"
className={classNames("mx_BaseAvatar", className)}
onClick={onClick}
inputRef={inputRef}
>
{textNode}
{imgNode}
</AccessibleButton>
);
} else {
return (
<span
className={classNames("mx_BaseAvatar", className)}
ref={inputRef}
{...otherProps}
role="presentation"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Losing this seems bad

>
{textNode}
{imgNode}
</span>
);
}
}

if (onClick) {
return (
<AccessibleButton
className={classNames("mx_BaseAvatar mx_BaseAvatar_image", className)}
element="img"
src={imageUrl}
onClick={onClick}
onError={onError}
style={{
width: toPx(width),
height: toPx(height),
}}
title={title}
alt={altText}
inputRef={inputRef}
data-testid="avatar-img"
{...otherProps}
/>
);
} else {
return (
<img
loading="lazy"
className={classNames("mx_BaseAvatar mx_BaseAvatar_image", className)}
src={imageUrl}
onError={onError}
style={{
width: toPx(width),
height: toPx(height),
}}
title={title}
alt=""
ref={inputRef}
data-testid="avatar-img"
{...otherProps}
/>
);
}
return (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like onClick is unused

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, not sure why ESLint was not shouting at me, back in use now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rule is disabled on this block

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<Avatar
ref={inputRef}
src={imageUrl}
id={idName ?? ""}
name={name ?? ""}
type={type}
size={`${width}px`}
className={classNames("mx_BaseAvatar", className)}
aria-label={altText}
onError={onError}
title={title}
{...otherProps}
/>
);
};

export default BaseAvatar;
Expand Down
1 change: 0 additions & 1 deletion src/components/views/avatars/MemberAvatar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,6 @@ export default function MemberAvatar({
: props.onClick
}
altText={_t("Profile picture")}
ariaLabel={_t("Profile picture")}
/>
);
}
Expand Down
5 changes: 1 addition & 4 deletions src/components/views/avatars/RoomAvatar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ limitations under the License.

import React, { ComponentProps } from "react";
import { Room, RoomStateEvent, MatrixEvent, EventType, RoomType } from "matrix-js-sdk/src/matrix";
import classNames from "classnames";

import BaseAvatar from "./BaseAvatar";
import ImageView from "../elements/ImageView";
Expand Down Expand Up @@ -140,9 +139,7 @@ export default class RoomAvatar extends React.Component<IProps, IState> {
return (
<BaseAvatar
{...otherProps}
className={classNames(className, {
mx_RoomAvatar_isSpaceRoom: (room?.getType() ?? this.props.oobData?.roomType) === RoomType.Space,
})}
type={(room?.getType() ?? this.props.oobData?.roomType) === RoomType.Space ? "square" : "round"}
name={roomName}
idName={this.roomIdName}
urls={this.state.urls}
Expand Down
2 changes: 1 addition & 1 deletion src/components/views/messages/RoomAvatarEvent.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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} />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this happen in a different PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

But it looks like it genuinely wants a 14px (OD) avatar?

</AccessibleButton>
),
},
Expand Down
35 changes: 24 additions & 11 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -1265,7 +1265,14 @@
resolved "https://registry.yarnpkg.com/@babel/regjsgen/-/regjsgen-0.8.0.tgz#f0ba69b075e1f05fb2825b7fad991e7adbb18310"
integrity sha512-x/rqGMdzj+fWZvCOYForTghzbtqPDZ5gPwaoNGHdgDfF2QA/XZbCBp4Moo5scrkAMPhB7z26XM/AaHuIJdgauA==

"@babel/runtime@^7.0.0", "@babel/runtime@^7.12.13", "@babel/runtime@^7.12.5", "@babel/runtime@^7.13.10", "@babel/runtime@^7.14.6", "@babel/runtime@^7.15.4", "@babel/runtime@^7.17.9", "@babel/runtime@^7.20.7", "@babel/runtime@^7.5.5", "@babel/runtime@^7.8.4", "@babel/runtime@^7.8.7", "@babel/runtime@^7.9.2":
"@babel/runtime@^7.0.0", "@babel/runtime@^7.12.13", "@babel/runtime@^7.12.5", "@babel/runtime@^7.14.6", "@babel/runtime@^7.15.4", "@babel/runtime@^7.17.9", "@babel/runtime@^7.20.7", "@babel/runtime@^7.5.5", "@babel/runtime@^7.8.4", "@babel/runtime@^7.8.7", "@babel/runtime@^7.9.2":
version "7.22.6"
resolved "https://registry.yarnpkg.com/@babel/runtime/-/runtime-7.22.6.tgz#57d64b9ae3cff1d67eb067ae117dac087f5bd438"
integrity sha512-wDb5pWm4WDdF6LFUde3Jl8WzPA+3ZbxYqkC6xAXuD3irdEHN1k0NfTRrJD8ZD378SJ61miMLCqIOXYhd8x+AJQ==
dependencies:
regenerator-runtime "^0.14.0"

"@babel/runtime@^7.13.10":
version "7.22.10"
resolved "https://registry.yarnpkg.com/@babel/runtime/-/runtime-7.22.10.tgz#ae3e9631fd947cb7e3610d3e9d8fef5f76696682"
integrity sha512-21t/fkKLMZI4pqP2wlmsQAWnYW1PDyKyyUV4vCi+B25ydmdaYTKXPwCj0BzSUnZf4seIiYvSA3jcZ3gdsMFkLQ==
Expand Down Expand Up @@ -2886,14 +2893,15 @@
dependencies:
svg2vectordrawable "^2.9.1"

"@vector-im/compound-web@^0.2.3":
version "0.2.3"
resolved "https://registry.yarnpkg.com/@vector-im/compound-web/-/compound-web-0.2.3.tgz#9dd4ed80109c614666103d05cd66723d1fad4d6c"
integrity sha512-7FI6Q1LN8dXur2sarP7UeMtAKcejuFw6AppM9Lu9fFjwLlbuIX2ZEprw1qa+EzgzUTysTU1TTdo7fxNqQwAQcA==
"@vector-im/compound-web@^0.2.4":
version "0.2.7"
resolved "https://registry.yarnpkg.com/@vector-im/compound-web/-/compound-web-0.2.7.tgz#146b353af0625b450616309d7c1cd8635cdd1bde"
integrity sha512-rqC6jcyKdNKnroqwN9GdmrmyBp0LEarzXkuucVPmCzsXZg5vSRo751oNngGQuDYx49ywLbKlv+B1/mgdCqC1Jw==
dependencies:
"@radix-ui/react-form" "^0.0.3"
classnames "^2.3.2"
lodash "^4.17.21"
graphemer "^1.4.0"
rimraf "^3.0.1"

abab@^2.0.6:
version "2.0.6"
Expand Down Expand Up @@ -8133,10 +8141,15 @@ regenerator-runtime@^0.14.0:
resolved "https://registry.yarnpkg.com/regenerator-runtime/-/regenerator-runtime-0.14.0.tgz#5e19d68eb12d486f797e15a3c6a918f7cec5eb45"
integrity sha512-srw17NI0TUWHuGa5CFGGmhfNIeja30WMBfbslPNhf6JrqQlLN5gcrvig1oqPxiVaXb0oW0XRKtH6Nngs5lKCIA==

regenerator-transform@^0.15.2:
version "0.15.2"
resolved "https://registry.yarnpkg.com/regenerator-transform/-/regenerator-transform-0.15.2.tgz#5bbae58b522098ebdf09bca2f83838929001c7a4"
integrity sha512-hfMp2BoF0qOk3uc5V20ALGDS2ddjQaLrdl7xrGXvAIow7qeWRM2VA2HuCHkUKk9slq3VwEwLNK3DFBqDfPGYtg==
regenerator-runtime@^0.14.0:
version "0.14.0"
resolved "https://registry.yarnpkg.com/regenerator-runtime/-/regenerator-runtime-0.14.0.tgz#5e19d68eb12d486f797e15a3c6a918f7cec5eb45"
integrity sha512-srw17NI0TUWHuGa5CFGGmhfNIeja30WMBfbslPNhf6JrqQlLN5gcrvig1oqPxiVaXb0oW0XRKtH6Nngs5lKCIA==

regenerator-transform@^0.15.1:
version "0.15.1"
resolved "https://registry.yarnpkg.com/regenerator-transform/-/regenerator-transform-0.15.1.tgz#f6c4e99fc1b4591f780db2586328e4d9a9d8dc56"
integrity sha512-knzmNAcuyxV+gQCufkYcvOqX/qIIfHLv0u5x79kRxuGojfYVky1f15TzZEu2Avte8QGepvUNTnLskf8E6X6Vyg==
dependencies:
"@babel/runtime" "^7.8.4"

Expand Down Expand Up @@ -8291,7 +8304,7 @@ rfdc@^1.3.0:
resolved "https://registry.yarnpkg.com/rfdc/-/rfdc-1.3.0.tgz#d0b7c441ab2720d05dc4cf26e01c89631d9da08b"
integrity sha512-V2hovdzFbOi77/WajaSMXk2OLm+xNIeQdMMuB7icj7bk6zi2F8GGAxigcnDFpJHbNyNcgyJDiP+8nOrY5cZGrA==

rimraf@^3.0.0, rimraf@^3.0.2:
rimraf@^3.0.0, rimraf@^3.0.1, rimraf@^3.0.2:
version "3.0.2"
resolved "https://registry.yarnpkg.com/rimraf/-/rimraf-3.0.2.tgz#f1a5402ba6220ad52cc1282bac1ae3aa49fd061a"
integrity sha512-JZkJMZkAGFFPP2YqXZXPbMlMBgsxzE8ILs4lMIX/2o0L9UBw9O/Y3o6wFw/i9YLapcUJWwqbi3kdxIPdC62TIA==
Expand Down