Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix jumpy timeline when the pinned message banner is displayed #28654

Merged
merged 1 commit into from
Dec 5, 2024
+86 −6
Merged

Fix jumpy timeline when the pinned message banner is displayed #28654

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 5 additions & 1 deletion src/components/structures/RoomView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2372,7 +2372,11 @@ export class RoomView extends React.Component<IRoomProps, IRoomState> {
);

const pinnedMessageBanner = (
<PinnedMessageBanner room={this.state.room} permalinkCreator={this.permalinkCreator} />
<PinnedMessageBanner
room={this.state.room}
permalinkCreator={this.permalinkCreator}
resizeNotifier={this.props.resizeNotifier}
/>
);

let messageComposer;
Expand Down
34 changes: 31 additions & 3 deletions src/components/views/rooms/PinnedMessageBanner.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@
* Please see LICENSE files in the repository root for full details.
*/

import React, { JSX, useEffect, useState } from "react";
import React, { JSX, useEffect, useRef, useState } from "react";
import PinIcon from "@vector-im/compound-design-tokens/assets/web/icons/pin-solid";
import { Button } from "@vector-im/compound-web";
import { Room } from "matrix-js-sdk/src/matrix";
import { MatrixEvent, Room } from "matrix-js-sdk/src/matrix";
import classNames from "classnames";

import { usePinnedEvents, useSortedFetchedPinnedEvents } from "../../../hooks/usePinnedEvents";
Expand All @@ -25,6 +25,7 @@ import { Action } from "../../../dispatcher/actions";
import MessageEvent from "../messages/MessageEvent";
import PosthogTrackers from "../../../PosthogTrackers.ts";
import { EventPreview } from "./EventPreview.tsx";
import ResizeNotifier from "../../../utils/ResizeNotifier";

/**
* The props for the {@link PinnedMessageBanner} component.
Expand All @@ -38,12 +39,20 @@ interface PinnedMessageBannerProps {
* The room where the banner is displayed
*/
room: Room;
/**
* The resize notifier to notify the timeline to resize itself when the banner is displayed or hidden.
*/
resizeNotifier: ResizeNotifier;
}

/**
* A banner that displays the pinned messages in a room.
*/
export function PinnedMessageBanner({ room, permalinkCreator }: PinnedMessageBannerProps): JSX.Element | null {
export function PinnedMessageBanner({
room,
permalinkCreator,
resizeNotifier,
}: PinnedMessageBannerProps): JSX.Element | null {
const pinnedEventIds = usePinnedEvents(room);
const pinnedEvents = useSortedFetchedPinnedEvents(room, pinnedEventIds);
const eventCount = pinnedEvents.length;
Expand All @@ -56,6 +65,8 @@ export function PinnedMessageBanner({ room, permalinkCreator }: PinnedMessageBan
}, [eventCount]);

const pinnedEvent = pinnedEvents[currentEventIndex];
useNotifyTimeline(pinnedEvent, resizeNotifier);

if (!pinnedEvent) return null;

const shouldUseMessageEvent = pinnedEvent.isRedacted() || pinnedEvent.isDecryptionFailure();
Expand Down Expand Up @@ -128,6 +139,23 @@ export function PinnedMessageBanner({ room, permalinkCreator }: PinnedMessageBan
);
}

/**
* When the banner is displayed or hidden, we want to notify the timeline to resize itself.
* @param pinnedEvent
* @param resizeNotifier
*/
function useNotifyTimeline(pinnedEvent: MatrixEvent | null, resizeNotifier: ResizeNotifier): void {
const previousEvent = useRef<MatrixEvent | null>(null);
useEffect(() => {
// If we switch from a pinned message to no pinned message or the opposite, we want to resize the timeline
if ((previousEvent.current && !pinnedEvent) || (!previousEvent.current && pinnedEvent)) {
resizeNotifier.notifyTimelineHeightChanged();
}

previousEvent.current = pinnedEvent;
}, [pinnedEvent, resizeNotifier]);
}

const MAX_INDICATORS = 3;

/**
Expand Down
52 changes: 50 additions & 2 deletions test/unit-tests/components/views/rooms/PinnedMessageBanner-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import RightPanelStore from "../../../../../src/stores/right-panel/RightPanelSto
import { RightPanelPhases } from "../../../../../src/stores/right-panel/RightPanelStorePhases";
import { UPDATE_EVENT } from "../../../../../src/stores/AsyncStore";
import { Action } from "../../../../../src/dispatcher/actions";
import ResizeNotifier from "../../../../../src/utils/ResizeNotifier.ts";

describe("<PinnedMessageBanner />", () => {
const userId = "@alice:server.org";
Expand All @@ -28,10 +29,12 @@ describe("<PinnedMessageBanner />", () => {
let mockClient: MatrixClient;
let room: Room;
let permalinkCreator: RoomPermalinkCreator;
let resizeNotifier: ResizeNotifier;
beforeEach(() => {
mockClient = stubClient();
room = new Room(roomId, mockClient, userId);
permalinkCreator = new RoomPermalinkCreator(room);
resizeNotifier = new ResizeNotifier();
jest.spyOn(dis, "dispatch").mockReturnValue(undefined);
});

Expand Down Expand Up @@ -77,7 +80,7 @@ describe("<PinnedMessageBanner />", () => {
*/
function renderBanner() {
return render(
<PinnedMessageBanner permalinkCreator={permalinkCreator} room={room} />,
<PinnedMessageBanner permalinkCreator={permalinkCreator} room={room} resizeNotifier={resizeNotifier} />,
withClientContextRenderOptions(mockClient),
);
}
Expand Down Expand Up @@ -145,7 +148,9 @@ describe("<PinnedMessageBanner />", () => {
event3.getId()!,
]);
jest.spyOn(pinnedEventHooks, "useSortedFetchedPinnedEvents").mockReturnValue([event1, event2, event3]);
rerender(<PinnedMessageBanner permalinkCreator={permalinkCreator} room={room} />);
rerender(
<PinnedMessageBanner permalinkCreator={permalinkCreator} room={room} resizeNotifier={resizeNotifier} />,
);
await expect(screen.findByText("Third pinned message")).resolves.toBeVisible();
expect(asFragment()).toMatchSnapshot();
});
Expand Down Expand Up @@ -206,6 +211,42 @@ describe("<PinnedMessageBanner />", () => {
expect(asFragment()).toMatchSnapshot();
});

describe("Notify the timeline to resize", () => {
beforeEach(() => {
jest.spyOn(resizeNotifier, "notifyTimelineHeightChanged");
jest.spyOn(pinnedEventHooks, "usePinnedEvents").mockReturnValue([event1.getId()!, event2.getId()!]);
jest.spyOn(pinnedEventHooks, "useSortedFetchedPinnedEvents").mockReturnValue([event1, event2]);
});

it("should notify the timeline to resize when we display the banner", async () => {
renderBanner();
await expect(screen.findByText("Second pinned message")).resolves.toBeVisible();
// The banner is displayed, so we need to resize the timeline
expect(resizeNotifier.notifyTimelineHeightChanged).toHaveBeenCalledTimes(1);

await userEvent.click(screen.getByRole("button", { name: "View the pinned message in the timeline." }));
await expect(screen.findByText("First pinned message")).resolves.toBeVisible();
// The banner is already displayed, so we don't need to resize the timeline
expect(resizeNotifier.notifyTimelineHeightChanged).toHaveBeenCalledTimes(1);
});

it("should notify the timeline to resize when we hide the banner", async () => {
const { rerender } = renderBanner();
await expect(screen.findByText("Second pinned message")).resolves.toBeVisible();
// The banner is displayed, so we need to resize the timeline
expect(resizeNotifier.notifyTimelineHeightChanged).toHaveBeenCalledTimes(1);

// The banner has no event to display and is hidden
jest.spyOn(pinnedEventHooks, "usePinnedEvents").mockReturnValue([]);
jest.spyOn(pinnedEventHooks, "useSortedFetchedPinnedEvents").mockReturnValue([]);
rerender(
<PinnedMessageBanner permalinkCreator={permalinkCreator} room={room} resizeNotifier={resizeNotifier} />,
);
// The timeline should be resized
expect(resizeNotifier.notifyTimelineHeightChanged).toHaveBeenCalledTimes(2);
});
});

describe("Right button", () => {
beforeEach(() => {
jest.spyOn(pinnedEventHooks, "usePinnedEvents").mockReturnValue([event1.getId()!, event2.getId()!]);
Expand All @@ -217,6 +258,8 @@ describe("<PinnedMessageBanner />", () => {
jest.spyOn(RightPanelStore.instance, "isOpenForRoom").mockReturnValue(false);

renderBanner();
await expect(screen.findByText("Second pinned message")).resolves.toBeVisible();

expect(screen.getByRole("button", { name: "View all" })).toBeVisible();
});

Expand All @@ -228,6 +271,8 @@ describe("<PinnedMessageBanner />", () => {
});

renderBanner();
await expect(screen.findByText("Second pinned message")).resolves.toBeVisible();

expect(screen.getByRole("button", { name: "View all" })).toBeVisible();
});

Expand All @@ -239,6 +284,8 @@ describe("<PinnedMessageBanner />", () => {
});

renderBanner();
await expect(screen.findByText("Second pinned message")).resolves.toBeVisible();

expect(screen.getByRole("button", { name: "Close list" })).toBeVisible();
});

Expand All @@ -263,6 +310,7 @@ describe("<PinnedMessageBanner />", () => {
});

renderBanner();
await expect(screen.findByText("Second pinned message")).resolves.toBeVisible();
expect(screen.getByRole("button", { name: "Close list" })).toBeVisible();

jest.spyOn(RightPanelStore.instance, "isOpenForRoom").mockReturnValue(false);
Expand Down
Loading