Tweak pill UI (#10417)

matrix-org · Mar 22, 2023 · 3eb6a55 · 3eb6a55
1 parent 4c2b18c
commit 3eb6a55
Show file tree

Hide file tree

Showing 8 changed files with 177 additions and 54 deletions.
diff --git a/res/css/views/elements/_Pill.pcss b/res/css/views/elements/_Pill.pcss
@@ -59,12 +59,6 @@ limitations under the License.
         min-width: $font-16px; /* ensure the avatar is not compressed */
     }
 
-    &.mx_EventPill .mx_BaseAvatar {
-        /* Event pill avatars are inside the text. */
-        margin-inline-start: 0.2em;
-        margin-inline-end: 0.2em;
-    }
-
     .mx_Pill_text {
         min-width: 0;
         overflow: hidden;

diff --git a/src/components/views/elements/Pill.tsx b/src/components/views/elements/Pill.tsx
@@ -45,6 +45,8 @@ export const pillRoomNotifLen = (): number => {
     return "@room".length;
 };
 
+const linkIcon = <LinkIcon className="mx_Pill_LinkIcon mx_BaseAvatar mx_BaseAvatar_image" />;
+
 const PillRoomAvatar: React.FC<{
     shouldShowPillAvatar: boolean;
     room: Room | null;
@@ -56,7 +58,7 @@ const PillRoomAvatar: React.FC<{
     if (room) {
         return <RoomAvatar room={room} width={16} height={16} aria-hidden="true" />;
     }
-    return <LinkIcon className="mx_Pill_LinkIcon mx_BaseAvatar mx_BaseAvatar_image" />;
+    return linkIcon;
 };
 
 const PillMemberAvatar: React.FC<{
@@ -88,7 +90,7 @@ export interface PillProps {
 
 export const Pill: React.FC<PillProps> = ({ type: propType, url, inMessage, room, shouldShowPillAvatar = true }) => {
     const [hover, setHover] = useState(false);
-    const { member, onClick, resourceId, targetRoom, text, type } = usePermalink({
+    const { event, member, onClick, resourceId, targetRoom, text, type } = usePermalink({
         room,
         type: propType,
         url,
@@ -116,35 +118,38 @@ export const Pill: React.FC<PillProps> = ({ type: propType, url, inMessage, room
     };
 
     const tip = hover && resourceId ? <Tooltip label={resourceId} alignment={Alignment.Right} /> : null;
-    let content: (ReactElement | string)[] = [];
-    const textElement = <span className="mx_Pill_text">{text}</span>;
+    let avatar: ReactElement | null = null;
+    let pillText: string | null = text;
 
     switch (type) {
         case PillType.EventInOtherRoom:
             {
-                const avatar = <PillRoomAvatar shouldShowPillAvatar={shouldShowPillAvatar} room={targetRoom} />;
-                content = [_t("Message in"), avatar || " ", textElement];
+                avatar = <PillRoomAvatar shouldShowPillAvatar={shouldShowPillAvatar} room={targetRoom} />;
+                pillText = _t("Message in %(room)s", {
+                    room: text,
+                });
             }
             break;
         case PillType.EventInSameRoom:
             {
-                const avatar = <PillMemberAvatar shouldShowPillAvatar={shouldShowPillAvatar} member={member} />;
-                content = [_t("Message from"), avatar || " ", textElement];
+                if (event) {
+                    avatar = <PillMemberAvatar shouldShowPillAvatar={shouldShowPillAvatar} member={member} />;
+                    pillText = _t("Message from %(user)s", {
+                        user: text,
+                    });
+                } else {
+                    avatar = linkIcon;
+                    pillText = _t("Message");
+                }
             }
             break;
         case PillType.AtRoomMention:
         case PillType.RoomMention:
         case "space":
-            {
-                const avatar = <PillRoomAvatar shouldShowPillAvatar={shouldShowPillAvatar} room={targetRoom} />;
-                content = [avatar, textElement];
-            }
+            avatar = <PillRoomAvatar shouldShowPillAvatar={shouldShowPillAvatar} room={targetRoom} />;
             break;
         case PillType.UserMention:
-            {
-                const avatar = <PillMemberAvatar shouldShowPillAvatar={shouldShowPillAvatar} member={member} />;
-                content = [avatar, textElement];
-            }
+            avatar = <PillMemberAvatar shouldShowPillAvatar={shouldShowPillAvatar} member={member} />;
             break;
         default:
             return null;
@@ -161,12 +166,14 @@ export const Pill: React.FC<PillProps> = ({ type: propType, url, inMessage, room
                         onMouseOver={onMouseOver}
                         onMouseLeave={onMouseLeave}
                     >
-                        {content}
+                        {avatar}
+                        <span className="mx_Pill_text">{pillText}</span>
                         {tip}
                     </a>
                 ) : (
                     <span className={classes} onMouseOver={onMouseOver} onMouseLeave={onMouseLeave}>
-                        {content}
+                        {avatar}
+                        <span className="mx_Pill_text">{pillText}</span>
                         {tip}
                     </span>
                 )}

diff --git a/src/hooks/usePermalink.ts b/src/hooks/usePermalink.ts
@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-import { Room, RoomMember } from "matrix-js-sdk/src/matrix";
+import { MatrixEvent, Room, RoomMember } from "matrix-js-sdk/src/matrix";
 
 import { ButtonEvent } from "../components/views/elements/AccessibleButton";
 import { PillType } from "../components/views/elements/Pill";
@@ -70,6 +70,11 @@ interface HookResult {
      * null here means that the type cannot be detected. Most likely if the URL was not a permalink.
      */
     type: PillType | "space" | null;
+    /**
+     * Target event of the permalink.
+     * Null if unable to load the event.
+     */
+    event: MatrixEvent | null;
 }
 
 /**
@@ -166,6 +171,7 @@ export const usePermalink: (args: Args) => HookResult = ({
     }
 
     return {
+        event,
         member,
         onClick,
         resourceId,

diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json
@@ -2592,8 +2592,8 @@
     "Rotate Right": "Rotate Right",
     "Information": "Information",
     "Language Dropdown": "Language Dropdown",
-    "Message in": "Message in",
-    "Message from": "Message from",
+    "Message in %(room)s": "Message in %(room)s",
+    "Message from %(user)s": "Message from %(user)s",
     "Create poll": "Create poll",
     "Create Poll": "Create Poll",
     "Edit poll": "Edit poll",

diff --git a/test/components/views/elements/__snapshots__/Pill-test.tsx.snap b/test/components/views/elements/__snapshots__/Pill-test.tsx.snap
@@ -71,7 +71,6 @@ exports[`<Pill> should render the expected pill for a message in another room 1`
         class="mx_Pill mx_EventPill"
         href="https://matrix.to/#/!room1:example.com/$123-456"
       >
-        Message in
         <span
           aria-hidden="true"
           class="mx_BaseAvatar"
@@ -96,7 +95,7 @@ exports[`<Pill> should render the expected pill for a message in another room 1`
         <span
           class="mx_Pill_text"
         >
-          Room 1
+          Message in Room 1
         </span>
       </a>
     </bdi>
@@ -112,7 +111,6 @@ exports[`<Pill> should render the expected pill for a message in the same room 1
         class="mx_Pill mx_EventPill"
         href="https://matrix.to/#/!room1:example.com/$123-456"
       >
-        Message from
         <span
           aria-hidden="true"
           class="mx_BaseAvatar"
@@ -137,7 +135,7 @@ exports[`<Pill> should render the expected pill for a message in the same room 1
         <span
           class="mx_Pill_text"
         >
-          User 1
+          Message from User 1
         </span>
       </a>
     </bdi>

diff --git a/test/components/views/messages/TextualBody-test.tsx b/test/components/views/messages/TextualBody-test.tsx
@@ -16,8 +16,9 @@ limitations under the License.
 
 import React from "react";
 import { MatrixClient, MatrixEvent } from "matrix-js-sdk/src/matrix";
-import { MockedObject } from "jest-mock";
+import { mocked, MockedObject } from "jest-mock";
 import { render } from "@testing-library/react";
+import * as prettier from "prettier";
 
 import { getMockClientWithEventEmitter, mkEvent, mkMessage, mkStubRoom } from "../../../test-utils";
 import { MatrixClientPeg } from "../../../../src/MatrixClientPeg";
@@ -28,10 +29,18 @@ import MatrixClientContext from "../../../../src/contexts/MatrixClientContext";
 import { RoomPermalinkCreator } from "../../../../src/utils/permalinks/Permalinks";
 import { MediaEventHelper } from "../../../../src/utils/MediaEventHelper";
 
-const mkRoomTextMessage = (body: string): MatrixEvent => {
+const room1Id = "!room1:example.com";
+const room2Id = "!room2:example.com";
+const room2Name = "Room 2";
+
+interface MkRoomTextMessageOpts {
+    roomId?: string;
+}
+
+const mkRoomTextMessage = (body: string, mkRoomTextMessageOpts?: MkRoomTextMessageOpts): MatrixEvent => {
     return mkMessage({
         msg: body,
-        room: "room_id",
+        room: mkRoomTextMessageOpts?.roomId ?? room1Id,
         user: "sender",
         event: true,
     });
@@ -42,7 +51,7 @@ const mkFormattedMessage = (body: string, formattedBody: string): MatrixEvent =>
         msg: body,
         formattedMsg: formattedBody,
         format: "org.matrix.custom.html",
-        room: "room_id",
+        room: room1Id,
         user: "sender",
         event: true,
     });
@@ -53,12 +62,29 @@ describe("<TextualBody />", () => {
         jest.spyOn(MatrixClientPeg, "get").mockRestore();
     });
 
-    const defaultRoom = mkStubRoom("room_id", "test room", undefined);
+    const defaultRoom = mkStubRoom(room1Id, "test room", undefined);
+    const otherRoom = mkStubRoom(room2Id, room2Name, undefined);
     let defaultMatrixClient: MockedObject<MatrixClient>;
+
+    const defaultEvent = mkEvent({
+        type: "m.room.message",
+        room: room1Id,
+        user: "sender",
+        content: {
+            body: "winks",
+            msgtype: "m.emote",
+        },
+        event: true,
+    });
+
     beforeEach(() => {
         defaultMatrixClient = getMockClientWithEventEmitter({
-            getRoom: () => defaultRoom,
-            getRooms: () => [defaultRoom],
+            getRoom: (roomId: string | undefined) => {
+                if (roomId === room1Id) return defaultRoom;
+                if (roomId === room2Id) return otherRoom;
+                return null;
+            },
+            getRooms: () => [defaultRoom, otherRoom],
             getAccountData: (): MatrixEvent | undefined => undefined,
             isGuest: () => false,
             mxcUrlToHttp: (s: string) => s,
@@ -67,18 +93,13 @@ describe("<TextualBody />", () => {
                 throw new Error("MockClient event not found");
             },
         });
-    });
 
-    const defaultEvent = mkEvent({
-        type: "m.room.message",
-        room: "room_id",
-        user: "sender",
-        content: {
-            body: "winks",
-            msgtype: "m.emote",
-        },
-        event: true,
+        mocked(defaultRoom).findEventById.mockImplementation((eventId: string) => {
+            if (eventId === defaultEvent.getId()) return defaultEvent;
+            return undefined;
+        });
     });
+
     const defaultProps = {
         mxEvent: defaultEvent,
         highlights: [] as string[],
@@ -88,6 +109,7 @@ describe("<TextualBody />", () => {
         permalinkCreator: new RoomPermalinkCreator(defaultRoom),
         mediaEventHelper: {} as MediaEventHelper,
     };
+
     const getComponent = (props = {}, matrixClient: MatrixClient = defaultMatrixClient, renderingFn?: any) =>
         (renderingFn ?? render)(
             <MatrixClientContext.Provider value={matrixClient}>
@@ -100,7 +122,7 @@ describe("<TextualBody />", () => {
 
         const ev = mkEvent({
             type: "m.room.message",
-            room: "room_id",
+            room: room1Id,
             user: "sender",
             content: {
                 body: "winks",
@@ -120,7 +142,7 @@ describe("<TextualBody />", () => {
 
         const ev = mkEvent({
             type: "m.room.message",
-            room: "room_id",
+            room: room1Id,
             user: "bot_sender",
             content: {
                 body: "this is a notice, probably from a bot",
@@ -196,13 +218,42 @@ describe("<TextualBody />", () => {
                 `"Visit <span><bdi><a class="mx_Pill mx_RoomPill" href="https://matrix.to/#/#room:example.com"><div class="mx_Pill_LinkIcon mx_BaseAvatar mx_BaseAvatar_image"></div><span class="mx_Pill_text">#room:example.com</span></a></bdi></span>"`,
             );
         });
+
+        it("should pillify a permalink to a message in the same room with the label »Message from Member«", () => {
+            const ev = mkRoomTextMessage(`Visit https://matrix.to/#/${room1Id}/${defaultEvent.getId()}`);
+            const { container } = getComponent({ mxEvent: ev });
+            const content = container.querySelector(".mx_EventTile_body");
+            expect(
+                prettier.format(content.innerHTML.replace(defaultEvent.getId(), "%event_id%"), {
+                    parser: "html",
+                }),
+            ).toMatchSnapshot();
+        });
+
+        it("should pillify a permalink to an unknown message in the same room with the label »Message«", () => {
+            const ev = mkRoomTextMessage(`Visit https://matrix.to/#/${room1Id}/!abc123`);
+            const { container } = getComponent({ mxEvent: ev });
+            const content = container.querySelector(".mx_EventTile_body");
+            expect(content).toMatchSnapshot();
+        });
+
+        it("should pillify a permalink to an event in another room with the label »Message in Room 2«", () => {
+            const ev = mkRoomTextMessage(`Visit https://matrix.to/#/${room2Id}/${defaultEvent.getId()}`);
+            const { container } = getComponent({ mxEvent: ev });
+            const content = container.querySelector(".mx_EventTile_body");
+            expect(
+                prettier.format(content.innerHTML.replace(defaultEvent.getId(), "%event_id%"), {
+                    parser: "html",
+                }),
+            ).toMatchSnapshot();
+        });
     });
 
     describe("renders formatted m.text correctly", () => {
         let matrixClient: MatrixClient;
         beforeEach(() => {
             matrixClient = getMockClientWithEventEmitter({
-                getRoom: () => mkStubRoom("room_id", "room name", undefined),
+                getRoom: () => mkStubRoom(room1Id, "room name", undefined),
                 getAccountData: (): MatrixEvent | undefined => undefined,
                 getUserId: () => "@me:my_server",
                 getHomeserverUrl: () => "https://my_server/",