Skip to content

Commit

Permalink
Fix outstanding UX issues with replies/mentions/keyword notifs
Browse files Browse the repository at this point in the history
  • Loading branch information
taffyko committed Oct 22, 2024
1 parent d4cf388 commit 3541391
Show file tree
Hide file tree
Showing 10 changed files with 121 additions and 12 deletions.
6 changes: 4 additions & 2 deletions res/css/views/elements/_Pill.pcss
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ Please see LICENSE files in the repository root for full details.
}

&.mx_UserPill_me,
&.mx_AtRoomPill {
&.mx_AtRoomPill,
&.mx_KeywordPill {
background-color: var(--cpd-color-bg-critical-primary) !important; /* To override .markdown-body */
}

Expand All @@ -45,7 +46,8 @@ Please see LICENSE files in the repository root for full details.
}

/* We don't want to indicate clickability */
&.mx_AtRoomPill:hover {
&.mx_AtRoomPill:hover,
&.mx_KeywordPill:hover {
background-color: var(--cpd-color-bg-critical-primary) !important; /* To override .markdown-body */
cursor: unset;
}
Expand Down
14 changes: 8 additions & 6 deletions res/css/views/rooms/_EventTile.pcss
Original file line number Diff line number Diff line change
Expand Up @@ -135,12 +135,6 @@ $left-gutter: 64px;
}
}

&.mx_EventTile_highlight,
&.mx_EventTile_highlight .markdown-body,
&.mx_EventTile_highlight .mx_EventTile_edited {
color: $alert;
}

&.mx_EventTile_bubbleContainer {
display: grid;
grid-template-columns: 1fr 100px;
Expand Down Expand Up @@ -584,6 +578,14 @@ $left-gutter: 64px;
padding-inline-start: calc(var(--EventTile_group_line-spacing-inline-start) + 20px);
}
}

&.mx_EventTile_highlight,
&.mx_EventTile_highlight .markdown-body {
> .mx_EventTile_line {
box-shadow: inset var(--EventTile-box-shadow-offset-x) 0 0 var(--EventTile-box-shadow-spread-radius)
$event-highlight-edge-color;
}
}
}

&[data-layout="bubble"] {
Expand Down
1 change: 1 addition & 0 deletions res/themes/dark/css/_dark.pcss
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ $spacePanel-bg-color: rgba(38, 39, 43, 0.82);
$panel-gradient: rgba(34, 38, 46, 0), rgba(34, 38, 46, 1);
$h3-color: $primary-content;
$event-highlight-bg-color: #25271f;
$event-highlight-edge-color: #f0b132;
$header-panel-text-primary-color: $text-secondary-color;
/* ******************** */

Expand Down
1 change: 1 addition & 0 deletions res/themes/legacy-dark/css/_legacy-dark.pcss
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ $widget-menu-bar-bg-color: $header-panel-bg-color;
$widget-body-bg-color: #1a1d23;

$event-highlight-bg-color: #25271f;
$event-highlight-edge-color: #f0b132;

/* event timestamp */
$event-timestamp-color: $text-secondary-color;
Expand Down
1 change: 1 addition & 0 deletions res/themes/legacy-light/css/_legacy-light.pcss
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ $widget-body-bg-color: #fff;
$yellow-background: #fff8e3;

$event-highlight-bg-color: $yellow-background;
$event-highlight-edge-color: var(--cpd-color-yellow-500);

/* event timestamp */
$event-timestamp-color: #acacac;
Expand Down
1 change: 1 addition & 0 deletions res/themes/light-custom/css/_custom.pcss
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ $button-danger-disabled-bg-color: var(--warning-color-50pct); /* still needs alp
/* --timeline-highlights-color */
$event-selected-color: var(--timeline-highlights-color);
$event-highlight-bg-color: var(--timeline-highlights-color);
$event-highlight-edge-color: var(--timeline-highlights-color);

/* redirect some variables away from their hardcoded values in the light theme */
$settings-grey-fg-color: $primary-content;
Expand Down
1 change: 1 addition & 0 deletions res/themes/light/css/_light.pcss
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ $spacePanel-bg-color: rgba(232, 232, 232, 0.77);
$panel-gradient: rgba(242, 245, 248, 0), rgba(242, 245, 248, 1);
$h3-color: #3d3b39;
$event-highlight-bg-color: $yellow-background;
$event-highlight-edge-color: var(--cpd-color-yellow-500);
$header-panel-text-primary-color: #91a1c0;
/* ******************** */

Expand Down
26 changes: 24 additions & 2 deletions src/components/views/elements/Pill.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ export enum PillType {
AtRoomMention = "TYPE_AT_ROOM_MENTION", // '@room' mention
EventInSameRoom = "TYPE_EVENT_IN_SAME_ROOM",
EventInOtherRoom = "TYPE_EVENT_IN_OTHER_ROOM",
Keyword = "TYPE_KEYWORD", // Used to highlight keywords that triggered a notification rule
}

export const pillRoomNotifPos = (text: string | null): number => {
Expand Down Expand Up @@ -76,14 +77,32 @@ export interface PillProps {
room?: Room;
// Whether to include an avatar in the pill
shouldShowPillAvatar?: boolean;
// Explicitly-provided text to display in the pill
text?: string;
}

export const Pill: React.FC<PillProps> = ({ type: propType, url, inMessage, room, shouldShowPillAvatar = true }) => {
const { event, member, onClick, resourceId, targetRoom, text, type } = usePermalink({
export const Pill: React.FC<PillProps> = ({
type: propType,
url,
inMessage,
room,
shouldShowPillAvatar = true,
text: customPillText,
}) => {
const {
event,
member,
onClick,
resourceId,
targetRoom,
text: linkText,
type,
} = usePermalink({
room,
type: propType,
url,
});
const text = customPillText ?? linkText;

if (!type || !text) {
return null;
Expand All @@ -96,6 +115,7 @@ export const Pill: React.FC<PillProps> = ({ type: propType, url, inMessage, room
mx_UserPill: type === PillType.UserMention,
mx_UserPill_me: resourceId === MatrixClientPeg.safeGet().getUserId(),
mx_EventPill: type === PillType.EventInOtherRoom || type === PillType.EventInSameRoom,
mx_KeywordPill: type === PillType.Keyword,
});

let avatar: ReactElement | null = null;
Expand Down Expand Up @@ -131,6 +151,8 @@ export const Pill: React.FC<PillProps> = ({ type: propType, url, inMessage, room
case PillType.UserMention:
avatar = <PillMemberAvatar shouldShowPillAvatar={shouldShowPillAvatar} member={member} />;
break;
case PillType.Keyword:
break;
default:
return null;
}
Expand Down
63 changes: 62 additions & 1 deletion src/components/views/messages/TextualBody.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@ Please see LICENSE files in the repository root for full details.

import React, { createRef, SyntheticEvent, MouseEvent } from "react";
import ReactDOM from "react-dom";
import { MsgType } from "matrix-js-sdk/src/matrix";
import { MsgType, PushRuleKind } from "matrix-js-sdk/src/matrix";
import { TooltipProvider } from "@vector-im/compound-web";
import { globToRegexp } from "matrix-js-sdk/src/utils";

import * as HtmlUtils from "../../../HtmlUtils";
import { formatDate } from "../../../DateUtils";
Expand All @@ -36,6 +37,7 @@ import { EditWysiwygComposer } from "../rooms/wysiwyg_composer";
import { IEventTileOps } from "../rooms/EventTile";
import { MatrixClientPeg } from "../../../MatrixClientPeg";
import CodeBlock from "./CodeBlock";
import { Pill, PillType } from "../elements/Pill";

interface IState {
// the URLs (if any) to be previewed with a LinkPreviewWidget inside this TextualBody.
Expand Down Expand Up @@ -102,6 +104,16 @@ export default class TextualBody extends React.Component<IBodyProps, IState> {
}
}
}

// Highlight notification keywords using pills
const pushDetails = this.props.mxEvent.getPushDetails();
if (
pushDetails.rule?.enabled &&
pushDetails.rule.kind === PushRuleKind.ContentSpecific &&
pushDetails.rule.pattern
) {
this.pillifyNotificationKeywords([content], this.regExpForKeywordPattern(pushDetails.rule.pattern));
}
}

private addCodeElement(pre: HTMLPreElement): void {
Expand Down Expand Up @@ -211,6 +223,55 @@ export default class TextualBody extends React.Component<IBodyProps, IState> {
}
}

/**
* Marks the text that activated a push-notification keyword pattern.
*/
private pillifyNotificationKeywords(nodes: ArrayLike<Element>, exp: RegExp): void {
let node: Node | null = nodes[0];
while (node) {
if (node.nodeType === Node.TEXT_NODE) {
const text = node.nodeValue;
if (!text) {
node = node.nextSibling;
continue;
}
const match = text.match(exp);
if (!match || match.length < 3) {
node = node.nextSibling;
continue;
}
const keywordText = match[2];
const idx = match.index! + match[1].length;
const before = text.substring(0, idx);
const after = text.substring(idx + keywordText.length);

const container = document.createElement("span");
const newContent = (
<>
{before}
<TooltipProvider>
<Pill text={keywordText} type={PillType.Keyword} />
</TooltipProvider>
{after}
</>
);
ReactDOM.render(newContent, container);

node.parentNode?.replaceChild(container, node);
} else if (node.childNodes && node.childNodes.length) {
this.pillifyNotificationKeywords(node.childNodes as NodeListOf<Element>, exp);
}

node = node.nextSibling;
}
}

private regExpForKeywordPattern(pattern: string): RegExp {
// Reflects the push notification pattern-matching implementation at
// https://github.com/matrix-org/matrix-js-sdk/blob/dbd7d26968b94700827bac525c39afff2c198e61/src/pushprocessor.ts#L570
return new RegExp("(^|\\W)(" + globToRegexp(pattern) + ")(\\W|$)", "i");
}

private findLinks(nodes: ArrayLike<Element>): string[] {
let links: string[] = [];

Expand Down
19 changes: 18 additions & 1 deletion test/unit-tests/components/views/messages/TextualBody-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ Please see LICENSE files in the repository root for full details.
*/

import React from "react";
import { MatrixClient, MatrixEvent } from "matrix-js-sdk/src/matrix";
import { MatrixClient, MatrixEvent, PushRuleKind } from "matrix-js-sdk/src/matrix";
import { mocked, MockedObject } from "jest-mock";
import { render, waitFor } from "jest-matrix-react";

Expand Down Expand Up @@ -228,6 +228,23 @@ describe("<TextualBody />", () => {
const content = container.querySelector(".mx_EventTile_body");
expect(content.innerHTML.replace(defaultEvent.getId(), "%event_id%")).toMatchSnapshot();
});

it("should pillify a keyword responsible for triggering a notification", () => {
const ev = mkRoomTextMessage("foo bar baz");
ev.setPushDetails(undefined, {
actions: [],
pattern: "bar",
rule_id: "bar",
default: false,
enabled: true,
kind: PushRuleKind.ContentSpecific,
});
const { container } = getComponent({ mxEvent: ev });
const content = container.querySelector(".mx_EventTile_body");
expect(content.innerHTML).toMatchInlineSnapshot(
`"<span>foo <bdi><span tabindex="0"><span class="mx_Pill mx_KeywordPill"><span class="mx_Pill_text">bar</span></span></span></bdi> baz</span>"`,
);
});
});

describe("renders formatted m.text correctly", () => {
Expand Down

0 comments on commit 3541391

Please sign in to comment.