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

When pinning or unpinning messages, link to the specific message#6489

Merged
turt2live merged 11 commits into
matrix-org:developfrom
psrpinto:better-pinned-messages
Sep 3, 2021
Merged

When pinning or unpinning messages, link to the specific message#6489
turt2live merged 11 commits into
matrix-org:developfrom
psrpinto:better-pinned-messages

Conversation

@psrpinto
Copy link
Copy Markdown
Contributor

@psrpinto psrpinto commented Jul 27, 2021

Fixes element-hq/element-web#17947

This PR makes it so that, whenever a single message is pinned or unpinned, a link to the message is supplied. Whenever more than one message is pinned/unpinned, it falls back to the previous behavior: Foo changed the pinned messages for the room.

Screenshot 2021-07-27 at 17 28 30

psrpinto added 3 commits July 27, 2021 17:14
@psrpinto
Add test for textForPinnedEvent
40ead34 
Signed-off-by: Paulo Pinto <paulo.pinto@automattic.com>
@psrpinto
When a single message is pinned, link to it
8fe7df9 
Signed-off-by: Paulo Pinto <paulo.pinto@automattic.com>
@psrpinto
When a single message is unpinned, link to it
3f2dadf 
Signed-off-by: Paulo Pinto <paulo.pinto@automattic.com>
@t3chguy t3chguy requested a review from a team July 27, 2021 16:37
@psrpinto
Copy link
Copy Markdown
Contributor Author

psrpinto commented Jul 27, 2021 •
edited
Loading

I'm not super happy with the code here since there's a lot of repetition happening and the textForPinnedEvent() function is getting out of hand. I think, maybe in a follow-up PR, this could be extracted to a component (though I'm not sure where that component would go).

I think another subject to consider would be the need for the allowJSX parameter, which introduces significant complexity. After chatting with @SimonBrandner, this parameter is needed since Notifications can only contain plain text. I think maybe we could look into generating the plain text from the JSX/HTML? I think that could be feasible, and would remove the need for the allowJSX parameter, and also the translations for the plain text versions.

Finally, something else to consider is that there's some shared concerns between the code in the textForPinnedEvent() function and the MemberEventListSummary, as evidenced in #6349. I think this maybe shows the need for a component, as I mention above, though it's maybe too early to consider those shared concerns.

psrpinto added 2 commits July 27, 2021 17:53
@psrpinto
Fix formatting
400e772 
Signed-off-by: Paulo Pinto <paulo.pinto@automattic.com>
@psrpinto
Add missing translations
6c945ee 
Signed-off-by: Paulo Pinto <paulo.pinto@automattic.com>
@turt2live
Copy link
Copy Markdown
Member

turt2live commented Jul 27, 2021

(if this is ready for review, please take it out of draft state - thanks!)

@psrpinto psrpinto marked this pull request as ready for review July 27, 2021 17:04
turt2live
turt2live suggested changes Aug 5, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

overall it looks great, just a couple points to cover!

Comment thread src/TextForEvent.tsx Outdated
Comment thread src/TextForEvent.tsx Outdated
Comment thread test/TextForEvent-test.ts Outdated
Comment thread test/__snapshots__/TextForEvent-test.ts.snap Outdated
@turt2live turt2live requested a review from a team August 5, 2021 21:34
@psrpinto
Collapse lines together to reduce line/indent spam
94d3dd6 
Signed-off-by: Paulo Pinto <paulo.pinto@automattic.com>
@psrpinto
Refactor tests
45fd3d8 
So that there's one top level `describe('TextForEvent')`, followed by a nested
`describe('textForPinnedEvent')`, containting all the `it()`s.

Signed-off-by: Paulo Pinto <paulo.pinto@automattic.com>
@psrpinto
Refactor tests so that snapshots aren't used
79cf69b 
Signed-off-by: Paulo Pinto <paulo.pinto@automattic.com>
@turt2live turt2live self-requested a review August 11, 2021 13:14
@psrpinto
Fix test case
ca8832f 
The test case is:
"mentions message when a single message was pinned, with multiple previously pinned messages"

However, the test case was also unpinning messages. That is now fixed.

Signed-off-by: Paulo Pinto <paulo.pinto@automattic.com>
@psrpinto
Change order of test cases
a51b114 
Just moving test cases so that "generic message" ones are grouped at the bottom.

Signed-off-by: Paulo Pinto <paulo.pinto@automattic.com>
@psrpinto
Handle case where one message is pinned, and another unpinned
aaeb996 
Signed-off-by: Paulo Pinto <paulo.pinto@automattic.com>
@psrpinto
Copy link
Copy Markdown
Contributor Author

psrpinto commented Aug 11, 2021

I believe I've addressed all remarks, this should be ready for re-review 🙏

turt2live
turt2live approved these changes Aug 23, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

Thanks! Here's hoping design gets to it soon 😅

@amshakal
Copy link
Copy Markdown

amshakal commented Aug 26, 2021

Designer is here to review and it looks good! 👯 👯

@psrpinto psrpinto requested a review from turt2live September 3, 2021 18:59
@turt2live turt2live merged commit 60e2d61 into matrix-org:develop Sep 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@turt2live turt2live Awaiting requested review from turt2live

2 more reviewers

@Palid Palid Palid left review comments

@amshakal amshakal amshakal approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Format pinned message changes to reflect the actual changes

4 participants

@psrpinto @turt2live @amshakal @Palid