From 3342aa5ff8ac27de52df96c0f1d21494e6a40976 Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 21 May 2024 11:37:02 +0100 Subject: [PATCH] Refactor some logic into common AvatarSetting component (#12544) * Refactor some logic into common AvatarSetting component We duplicated some of the logic of setting avatars between profiles & rooms. This pulls some of that logic into the AvatarSetting component and hopefully make things a little simpler. * Unsed import * Convert JS based hover to CSS * Remove unnecessary container * Test avatar-as-file path * Test file upload * Unused imports * Add test for RoomProfileSettings * Test removing room avatar * Move upload control CSS too * Remove commented code Co-authored-by: Florian Duros * Prettier * Coments & move style to inline as per PR suggestion * Better test names Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Fix test Upload input doesn't have that class anymore --------- Co-authored-by: Florian Duros Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- .../general-user-settings-tab.spec.ts | 4 +- res/css/views/settings/_AvatarSetting.pcss | 7 +- res/css/views/settings/_ProfileSettings.pcss | 4 - .../room_settings/RoomProfileSettings.tsx | 82 ++++------- .../views/settings/AvatarSetting.tsx | 127 +++++++++++++----- .../views/settings/ProfileSettings.tsx | 78 ++++------- .../RoomProfileSettings-test.tsx | 105 +++++++++++++++ .../views/settings/AvatarSetting-test.tsx | 53 ++++++-- 8 files changed, 296 insertions(+), 164 deletions(-) create mode 100644 test/components/views/room_settings/RoomProfileSettings-test.tsx diff --git a/playwright/e2e/settings/general-user-settings-tab.spec.ts b/playwright/e2e/settings/general-user-settings-tab.spec.ts index 625f1d6bd5..ae3718aba9 100644 --- a/playwright/e2e/settings/general-user-settings-tab.spec.ts +++ b/playwright/e2e/settings/general-user-settings-tab.spec.ts @@ -133,9 +133,7 @@ test.describe("General user settings tab", () => { test("should support adding and removing a profile picture", async ({ uut }) => { const profileSettings = uut.locator(".mx_ProfileSettings"); // Upload a picture - await profileSettings - .locator(".mx_ProfileSettings_avatarUpload") - .setInputFiles("playwright/sample-files/riot.png"); + await profileSettings.getByAltText("Upload").setInputFiles("playwright/sample-files/riot.png"); // Find and click "Remove" link button await profileSettings.locator(".mx_ProfileSettings_profile").getByRole("button", { name: "Remove" }).click(); diff --git a/res/css/views/settings/_AvatarSetting.pcss b/res/css/views/settings/_AvatarSetting.pcss index 98bf3ab9b8..a6d70a697a 100644 --- a/res/css/views/settings/_AvatarSetting.pcss +++ b/res/css/views/settings/_AvatarSetting.pcss @@ -23,6 +23,7 @@ limitations under the License. .mx_AvatarSetting_hover { transition: opacity var(--hover-transition); + opacity: 0; /* position to place the hover bg over the entire thing */ position: absolute; @@ -50,14 +51,10 @@ limitations under the License. } } - &.mx_AvatarSetting_avatar_hovering .mx_AvatarSetting_hover { + &.mx_AvatarSetting_avatarDisplay:hover .mx_AvatarSetting_hover { opacity: 1; } - &:not(.mx_AvatarSetting_avatar_hovering) .mx_AvatarSetting_hover { - opacity: 0; - } - & > * { box-sizing: border-box; } diff --git a/res/css/views/settings/_ProfileSettings.pcss b/res/css/views/settings/_ProfileSettings.pcss index 5caff1f2c0..73cdcd75c8 100644 --- a/res/css/views/settings/_ProfileSettings.pcss +++ b/res/css/views/settings/_ProfileSettings.pcss @@ -17,10 +17,6 @@ limitations under the License. .mx_ProfileSettings { border-bottom: 1px solid $quinary-content; - .mx_ProfileSettings_avatarUpload { - display: none; - } - .mx_ProfileSettings_profile { display: flex; diff --git a/src/components/views/room_settings/RoomProfileSettings.tsx b/src/components/views/room_settings/RoomProfileSettings.tsx index 15d03a4c5d..9e7183b33a 100644 --- a/src/components/views/room_settings/RoomProfileSettings.tsx +++ b/src/components/views/room_settings/RoomProfileSettings.tsx @@ -1,5 +1,5 @@ /* -Copyright 2019 New Vector Ltd +Copyright 2019, 2024 New Vector Ltd Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -21,11 +21,9 @@ import { EventType } from "matrix-js-sdk/src/matrix"; import { _t } from "../../../languageHandler"; import { MatrixClientPeg } from "../../../MatrixClientPeg"; import Field from "../elements/Field"; -import { mediaFromMxc } from "../../../customisations/Media"; import AccessibleButton, { ButtonEvent } from "../elements/AccessibleButton"; import AvatarSetting from "../settings/AvatarSetting"; import { htmlSerializeFromMdIfNeeded } from "../../../editor/serialize"; -import { chromeFileInputFix } from "../../../utils/BrowserWorkarounds"; interface IProps { roomId: string; @@ -35,8 +33,9 @@ interface IState { originalDisplayName: string; displayName: string; originalAvatarUrl: string | null; - avatarUrl: string | null; avatarFile: File | null; + // If true, the user has indicated that they wish to remove the avatar and this should happen on save. + avatarRemovalPending: boolean; originalTopic: string; topic: string; profileFieldsTouched: Record; @@ -57,8 +56,7 @@ export default class RoomProfileSettings extends React.Component if (!room) throw new Error(`Expected a room for ID: ${props.roomId}`); const avatarEvent = room.currentState.getStateEvents(EventType.RoomAvatar, ""); - let avatarUrl = avatarEvent?.getContent()["url"] ?? null; - if (avatarUrl) avatarUrl = mediaFromMxc(avatarUrl).getSquareThumbnailHttp(96); + const avatarUrl = avatarEvent?.getContent()["url"] ?? null; const topicEvent = room.currentState.getStateEvents(EventType.RoomTopic, ""); const topic = topicEvent && topicEvent.getContent() ? topicEvent.getContent()["topic"] : ""; @@ -71,8 +69,8 @@ export default class RoomProfileSettings extends React.Component originalDisplayName: name, displayName: name, originalAvatarUrl: avatarUrl, - avatarUrl: avatarUrl, avatarFile: null, + avatarRemovalPending: false, originalTopic: topic, topic: topic, profileFieldsTouched: {}, @@ -82,16 +80,23 @@ export default class RoomProfileSettings extends React.Component }; } - private uploadAvatar = (): void => { - this.avatarUpload.current?.click(); + private onAvatarChanged = (file: File): void => { + this.setState({ + avatarFile: file, + avatarRemovalPending: false, + profileFieldsTouched: { + ...this.state.profileFieldsTouched, + avatar: true, + }, + }); }; private removeAvatar = (): void => { // clear file upload field so same file can be selected if (this.avatarUpload.current) this.avatarUpload.current.value = ""; this.setState({ - avatarUrl: null, avatarFile: null, + avatarRemovalPending: true, profileFieldsTouched: { ...this.state.profileFieldsTouched, avatar: true, @@ -112,8 +117,8 @@ export default class RoomProfileSettings extends React.Component profileFieldsTouched: {}, displayName: this.state.originalDisplayName, topic: this.state.originalTopic, - avatarUrl: this.state.originalAvatarUrl, avatarFile: null, + avatarRemovalPending: false, }); }; @@ -138,11 +143,12 @@ export default class RoomProfileSettings extends React.Component if (this.state.avatarFile) { const { content_uri: uri } = await client.uploadContent(this.state.avatarFile); await client.sendStateEvent(this.props.roomId, EventType.RoomAvatar, { url: uri }, ""); - newState.avatarUrl = mediaFromMxc(uri).getSquareThumbnailHttp(96); - newState.originalAvatarUrl = newState.avatarUrl; + newState.originalAvatarUrl = uri; newState.avatarFile = null; - } else if (this.state.originalAvatarUrl !== this.state.avatarUrl) { + } else if (this.state.avatarRemovalPending) { await client.sendStateEvent(this.props.roomId, EventType.RoomAvatar, {}, ""); + newState.avatarRemovalPending = false; + newState.originalAvatarUrl = null; } if (this.state.originalTopic !== this.state.topic) { @@ -192,34 +198,6 @@ export default class RoomProfileSettings extends React.Component } }; - private onAvatarChanged = (e: React.ChangeEvent): void => { - if (!e.target.files || !e.target.files.length) { - this.setState({ - avatarUrl: this.state.originalAvatarUrl, - avatarFile: null, - profileFieldsTouched: { - ...this.state.profileFieldsTouched, - avatar: false, - }, - }); - return; - } - - const file = e.target.files[0]; - const reader = new FileReader(); - reader.onload = (ev) => { - this.setState({ - avatarUrl: String(ev.target?.result), - avatarFile: file, - profileFieldsTouched: { - ...this.state.profileFieldsTouched, - avatar: true, - }, - }); - }; - reader.readAsDataURL(file); - }; - public render(): React.ReactNode { let profileSettingsButtons; if (this.state.canSetName || this.state.canSetTopic || this.state.canSetAvatar) { @@ -241,14 +219,6 @@ export default class RoomProfileSettings extends React.Component return (
-
/>
{profileSettingsButtons} diff --git a/src/components/views/settings/AvatarSetting.tsx b/src/components/views/settings/AvatarSetting.tsx index 4aaadc2acf..875c1f9c93 100644 --- a/src/components/views/settings/AvatarSetting.tsx +++ b/src/components/views/settings/AvatarSetting.tsx @@ -1,5 +1,5 @@ /* -Copyright 2019 The Matrix.org Foundation C.I.C. +Copyright 2019, 2024 The Matrix.org Foundation C.I.C. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -14,51 +14,102 @@ See the License for the specific language governing permissions and limitations under the License. */ -import React, { useRef, useState } from "react"; -import classNames from "classnames"; +import React, { createRef, useCallback, useEffect, useRef, useState } from "react"; import { _t } from "../../../languageHandler"; -import AccessibleButton, { ButtonEvent } from "../elements/AccessibleButton"; +import AccessibleButton from "../elements/AccessibleButton"; +import { mediaFromMxc } from "../../../customisations/Media"; +import { chromeFileInputFix } from "../../../utils/BrowserWorkarounds"; interface IProps { - avatarUrl?: string; - avatarName: string; // name of user/room the avatar belongs to - uploadAvatar?: (e: ButtonEvent) => void; - removeAvatar?: (e: ButtonEvent) => void; + /** + * The current value of the avatar URL, as an mxc URL or a File. + * Generally, an mxc URL would be specified until the user selects a file, then + * the file supplied by the onChange callback would be supplied here until it's + * saved. + */ + avatar?: string | File; + + /** + * If true, the user cannot change the avatar + */ + disabled?: boolean; + + /** + * Called when the user has selected a new avatar + * The callback is passed a File object for the new avatar data + */ + onChange?: (f: File) => void; + + /** + * Called when the user wishes to remove the avatar + */ + removeAvatar?: () => void; + + /** + * The alt text for the avatar + */ avatarAltText: string; } -const AvatarSetting: React.FC = ({ avatarUrl, avatarAltText, avatarName, uploadAvatar, removeAvatar }) => { - const [isHovering, setIsHovering] = useState(false); - const hoveringProps = { - onMouseEnter: () => setIsHovering(true), - onMouseLeave: () => setIsHovering(false), - }; +/** + * Component for setting or removing an avatar on something (eg. a user or a room) + */ +const AvatarSetting: React.FC = ({ avatar, avatarAltText, onChange, removeAvatar, disabled }) => { + const fileInputRef = createRef(); + + // Real URL that we can supply to the img element, either a data URL or whatever mediaFromMxc gives + // This represents whatever avatar the user has chosen at the time + const [avatarURL, setAvatarURL] = useState(undefined); + useEffect(() => { + if (avatar instanceof File) { + const reader = new FileReader(); + reader.onload = () => { + setAvatarURL(reader.result as string); + }; + reader.readAsDataURL(avatar); + } else if (avatar) { + setAvatarURL(mediaFromMxc(avatar).getSquareThumbnailHttp(96) ?? undefined); + } else { + setAvatarURL(undefined); + } + }, [avatar]); + // TODO: Use useId() as soon as we're using React 18. // Prevents ID collisions when this component is used more than once on the same page. const a11yId = useRef(`hover-text-${Math.random()}`); + const onFileChanged = useCallback( + (e: React.ChangeEvent) => { + if (e.target.files) onChange?.(e.target.files[0]); + }, + [onChange], + ); + + const uploadAvatar = useCallback((): void => { + fileInputRef.current?.click(); + }, [fileInputRef]); + let avatarElement = ( ); - if (avatarUrl) { + if (avatarURL) { avatarElement = ( ); } @@ -67,17 +118,27 @@ const AvatarSetting: React.FC = ({ avatarUrl, avatarAltText, avatarName, if (uploadAvatar) { // insert an empty div to be the host for a css mask containing the upload.svg uploadAvatarBtn = ( - + <> + + + ); } let removeAvatarBtn: JSX.Element | undefined; - if (avatarUrl && removeAvatar) { + if (avatarURL && removeAvatar && !disabled) { removeAvatarBtn = ( {_t("action|remove")} @@ -85,16 +146,12 @@ const AvatarSetting: React.FC = ({ avatarUrl, avatarAltText, avatarName, ); } - const avatarClasses = classNames({ - mx_AvatarSetting_avatar: true, - mx_AvatarSetting_avatar_hovering: isHovering && uploadAvatar, - }); return ( -
+
{avatarElement}