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

MSC3531 - Implementing message hiding pending moderation #7518

Merged
merged 7 commits into from
Jan 17, 2022

Conversation

Yoric
Copy link
Contributor

@Yoric Yoric commented Jan 12, 2022

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
Copy link
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 pretty good to me! Some minor things, but overall should be good to go
The CI looks unhappy

res/css/views/messages/_HiddenBody.scss Outdated Show resolved Hide resolved
src/components/structures/TimelinePanel.tsx Outdated Show resolved Hide resolved
src/components/views/messages/HiddenBody.tsx Outdated Show resolved Hide resolved
src/components/views/messages/TextualBody.tsx Outdated Show resolved Hide resolved
@Yoric
Copy link
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
Contributor

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
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
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
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
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
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)

Copy link
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
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
Contributor

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
Contributor

@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
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
Member

@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
3 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants