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

Display pinned messages on a banner at the top of a room #12917

Merged
merged 13 commits into from
Aug 29, 2024

Conversation

florianduros
Copy link
Contributor

@florianduros florianduros commented Aug 22, 2024

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • Sign-off given on the changes (see CONTRIBUTING.md).

Part of https://github.com/element-hq/element-internal/issues/609
Figma https://www.figma.com/design/nwbnk8RaZctjrrRGTUcx1X/Pinned-Messages?node-id=24-126291&t=21A0TwvtjtfbAidQ-0

  • Extract the pinned event hooks of the pinned message list to a dedicated hook files
  • Add a banner at the top of a room to display the pinned messages.

@florianduros florianduros added the T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements label Aug 22, 2024
@florianduros florianduros force-pushed the florianduros/pinned-messages/banner branch from a6bb865 to f341ee2 Compare August 22, 2024 13:33
@florianduros florianduros force-pushed the florianduros/pinned-messages/banner branch from f341ee2 to 23b39af Compare August 22, 2024 16:46
@florianduros florianduros force-pushed the florianduros/pinned-messages/banner branch from 5c26f96 to 0e3c560 Compare August 26, 2024 08:29
@florianduros florianduros force-pushed the florianduros/pinned-messages/banner branch from 0e3c560 to 1f9dde9 Compare August 26, 2024 09:03
@florianduros florianduros force-pushed the florianduros/pinned-messages/banner branch from 1f9dde9 to b5895b4 Compare August 26, 2024 09:33
@florianduros florianduros force-pushed the florianduros/pinned-messages/banner branch from b5895b4 to 6ce4cc8 Compare August 27, 2024 08:43
@florianduros florianduros force-pushed the florianduros/pinned-messages/banner branch from a8f359e to 809790d Compare August 29, 2024 07:52
src/components/views/rooms/PinnedMessageBanner.tsx Outdated Show resolved Hide resolved
src/components/views/rooms/PinnedMessageBanner.tsx Outdated Show resolved Hide resolved
}

// Handle poll events
await room.processPollEvents([event]);
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, really feels like this is pulling logic out of the js-sdk and duplicating it here. Is there any way we could let the js-sdk do all this work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I copy pasta this logic from the previous impl of pinning. You are talking about the manual event fetching right?

Copy link
Contributor Author

@florianduros florianduros Aug 29, 2024

Choose a reason for hiding this comment

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

@dbkr My proposition is leave this duplication here since the duplication existed before this PR. And I'll make a dedicated PR to move all the ( https://github.com/matrix-org/matrix-react-sdk/blob/develop/src/components/views/messages/MPollEndBody.tsx#L41 ) duplicates into the js-sdk.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, sounds ideal if you have the time - thank you!

test/components/views/rooms/PinnedMessageBanner-test.tsx Outdated Show resolved Hide resolved
@florianduros florianduros added this pull request to the merge queue Aug 29, 2024
Merged via the queue into develop with commit d16ab09 Aug 29, 2024
29 checks passed
@florianduros florianduros deleted the florianduros/pinned-messages/banner branch August 29, 2024 14:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants