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

MSC3531 - Implementing message hiding pending moderation#7518

Merged
Yoric merged 7 commits intomatrix-org:developfrom
Yoric:yoric/msc3531
Jan 17, 2022
Merged

MSC3531 - Implementing message hiding pending moderation#7518
Yoric merged 7 commits intomatrix-org:developfrom
Yoric:yoric/msc3531

Conversation

@Yoric
Copy link
Copy Markdown
Contributor

@Yoric Yoric commented Jan 12, 2022 •
edited by github-actions Bot
Loading

Notes: Implementing message hiding pending moderation (MSC3531)

This is a followup to matrix-org/matrix-js-sdk#2041 .

This PR currently has no changelog labels, so will not be included in changelogs.

A reviewer can add one of: T-Deprecation, T-Enhancement, T-Defect, T-Task to indicate what type of change this is, or add Type: [enhancement/defect/task] to the description and I'll add them for you.

Preview: https://61dff9bc6685e816cdf7d5f6--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.

@Yoric Yoric requested a review from a team as a code owner January 12, 2022 11:15
@Yoric Yoric force-pushed the yoric/msc3531 branch 3 times, most recently from 41419f6 to ff32186 Compare January 12, 2022 11:45
@Yoric
MSC3531 - Implementing message hiding pending moderation
1b2b353 
Signed-off-by: David Teller <davidt@element.io>
germain-gg
germain-gg reviewed Jan 12, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@germain-gg germain-gg left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks pretty good to me! Some minor things, but overall should be good to go
The CI looks unhappy

Comment thread res/css/views/messages/_HiddenBody.scss Outdated
Comment thread src/components/structures/TimelinePanel.tsx Outdated
Comment thread src/components/views/messages/HiddenBody.tsx Outdated
Comment thread src/components/views/messages/TextualBody.tsx
@Yoric
Copy link
Copy Markdown
Contributor Author

Yoric commented Jan 12, 2022

Looks pretty good to me! Some minor things, but overall should be good to go The CI looks unhappy

What does it mean when the i18n tests fail with "files do not match"?

@germain-gg
Copy link
Copy Markdown
Contributor

germain-gg commented Jan 12, 2022

Not entirely, I have not encountered this issue before, but running yarn i18n yielded

ERROR: src/components/views/messages/TextualBody.tsx:517 Message pending moderation: %(reason)
Error: Invalid format specifier: '%(reason)'

@Yoric
Copy link
Copy Markdown
Contributor Author

Yoric commented Jan 12, 2022

Not entirely, I have not encountered this issue before, but running yarn i18n yielded

ERROR: src/components/views/messages/TextualBody.tsx:517 Message pending moderation: %(reason)
Error: Invalid format specifier: '%(reason)'

That is really odd, it doesn't show this message on my machine. What it does is remove deprecated(?) strings and reorder them.

@Yoric
Copy link
Copy Markdown
Contributor Author

Yoric commented Jan 12, 2022

Not entirely, I have not encountered this issue before, but running yarn i18n yielded

ERROR: src/components/views/messages/TextualBody.tsx:517 Message pending moderation: %(reason)
Error: Invalid format specifier: '%(reason)'

That is really odd, it doesn't show this message on my machine. What it does is remove deprecated(?) strings and reorder them.

Actually, it's even odder, in the patch, this string is Message pending moderation: %(reason)s (note the final "s").

@Yoric
Copy link
Copy Markdown
Contributor Author

Yoric commented Jan 12, 2022

Apparently, I was running some deprecated scripts locally. After nuking my node_modules and a yarn install --force, it works better!

@Yoric Yoric requested a review from germain-gg January 13, 2022 09:49
@Yoric
Copy link
Copy Markdown
Contributor Author

Yoric commented Jan 13, 2022

I have pushed an additional patch as I realized that I wasn't handling "reply to" correctly. This should now work.

@Yoric
Copy link
Copy Markdown
Contributor Author

Yoric commented Jan 13, 2022

Time for screenshots!

Message author

Screenshot: message author (should see their own messages as pending moderation)

Moderator

Screenshot: moderator (should see the message as pending moderation)

Witness

A message is pending moderation

Screenshot: witness (should not see the messages pending moderation)

Attempting to respond to a message pending moderation

Screenshot: witness attempting to reply (should not see the messages pending moderation)

germain-gg
germain-gg approved these changes Jan 14, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@germain-gg germain-gg left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@Yoric
Copy link
Copy Markdown
Contributor Author

Yoric commented Jan 17, 2022

Thanks for the reviews!
@gsouquet Do we need any additional UX review for this?

@germain-gg germain-gg requested a review from janogarcia January 17, 2022 13:56
@germain-gg
Copy link
Copy Markdown
Contributor

germain-gg commented Jan 17, 2022

Re-using the message deleted styles as you did looks appropriate to me, but I've assigned @janogarcia as a reviewer for a last sanity check.
Probably good to merge and we can still adjust based on feedback at a later stage

@Yoric Yoric merged commit 6b870ba into matrix-org:develop Jan 17, 2022
@janogarcia
Copy link
Copy Markdown
Contributor

janogarcia commented Jan 28, 2022

@gsouquet @Yoric Did we reuse any design system color for the message text and icon? The blue shades in the screenshots don't look familiar to me.

@Yoric
Copy link
Copy Markdown
Contributor Author

Yoric commented Jan 29, 2022

@gsouquet @Yoric Did we reuse any design system color for the message text and icon? The blue shades in the screenshots don't look familiar to me.

I didn't introduce anything new. I'm running both Firefox and Element Web on dark themes, I guess this may have an impact.

@turt2live
Copy link
Copy Markdown
Member

turt2live commented Jan 31, 2022

@Yoric looks like this is missing from the labs.md on the element-web repo - can we get a small PR to add it please? 😇

@nmscode nmscode mentioned this pull request Jul 26, 2023
Closed
3 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@janogarcia janogarcia Awaiting requested review from janogarcia

1 more reviewer

@germain-gg germain-gg germain-gg 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.

4 participants

@Yoric @germain-gg @janogarcia @turt2live