-
-
Notifications
You must be signed in to change notification settings - Fork 833
Conversation
Thank you for your contribution! I think this is going to need a product review to decide if this is something we want |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM codewise. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EmojiPicker
is actually used in two different places: the emoji picker in the composer, and the reaction picker in the timeline. Currently this change applies to both of them, though presumably it should only apply to the reaction picker.
Fixed in 7d335a5. I struggled with the naming here a little, and settled for |
@@ -27,6 +27,7 @@ import Preview from "./Preview"; | |||
import QuickReactions from "./QuickReactions"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This encourages creating more detailed, personalized reactions, so maybe there should be a reminder in the menu that reactions are never encrypted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code-wise LGTM
@@ -35,6 +36,7 @@ export const EMOJIS_PER_ROW = 8; | |||
const ZERO_WIDTH_JOINER = "\u200D"; | |||
|
|||
interface IProps { | |||
allowUnlisted?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
allowFreeform maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was on the fence here – "freeform emojis" are not a thing (and this is an EmojiPicker), but at the same time "unlisted emojis" are not the point here – it allows for just not using emojis at all (though technically that also allows for not-yet-supported, thus "unlisted" emojis). I'm indifferent, we can go either way.
Hey @tadzik - nice PR. Can you create a |
Signed-off-by: Tadeusz Sośnierz <tadeusz@sosnierz.com>
7d335a5
to
215df55
Compare
Yep: done in element-hq/element-web#19409. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
echoing approval to fix internal stats
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a product POV: while we don't want to add maintenance burden, this is a feature already supported by other apps and the MSC2677 currently implies that emojis are just one type of reaction that could be sent.
We should add the feature because:
- MSC2677 supports reactions with long
keys
, i.e. text reactions - This feature is already available through other apps and our users can see those reactions
- It is already possible to send freeform reactions through
/devtools
and bots from Element, so at the moment the feature exists but is obscure - Element users can already "+1" a freeform reaction on any platform by clicking it
Product considerations:
- If we're adding this feature on web, we should add it on mobile as well to maintain feature parity
Design considerations:
- How might we allow users to send freeform reactions
- How might we display long reactions in a user friendly way? (I can't see any upper limit set by the MSC)
- How might reporting of reactions look like?
- How might removal of reactions look like for room moderators?
Moderation considerations to address:
- Should reactions be reportable or can they be reported using the existing mechanism for reporting messages? @jimmackenzie maybe you can help with input on this one?
Please reach out to @frakic before merging the PR so that he can test the build and add test cases for RC testing.
I've used freeform reactions elsewhere, and loved them. From a safety perspective:
Recommendation: We should make it easy for clients to report reaction events as part of this change.
Recommendation: Update clients to allow room moderators to remove reactions. T&S to update safety tooling to handle those redactions as well (either via freshly-exposed event permalink for reactions, or via the reaction event-id).
Recommendation: Establish an upper limit for reaction length in the spec + implement in the clients. |
Other clients, at least FluffyChat and Nheko allow you to reply to a message with
FluffyChat cuts the reaction at some point (I am not sure what logic it uses) after which the full reaction text becomes visible by long touching (mobile) which also shows who have added that reaction. I think Element Web could make it visible on hover. |
* Render Beeper-style image emoji * Allow sending free-form reactions (matrix-org#6628) * Notify even when not focused
* Render Beeper-style image emoji * Allow sending free-form reactions (matrix-org#6628) * Notify even when not focused
This has been blocked for a very long time now. Given it's obvious user value, I propose us to be more pragmatic in getting it landed and not let perfect be the enemy of good. Summarizing the outstanding concerns that I've found:
@daniellekirkwood are you in agreement with the above? If so, could you help figure out the language for 1.? @tadzik could you look inot fixing 4.? |
I agree with all @Johennes said above, and I would further comment that the problems with length and reporting of reactions already exist in Element because we already display free-form reactions. |
Hi there! I really appreciate the hard-work and thought that's gone into this issue, along with the obvious passion that y'all feel for making this product more flexible and delightful. On behalf of the product team, I'm going to decline this PR. Here's a brief overview of why:
Apologies that I'm here with bad news, friends. Hope you have a great day 💛 |
It really doesn't. It is far more effort to do this by hand than using the Element Web console or using curl, both of these latter options can be automated in a while loop to go infinitely if you wish to spam. Element web still shows these, so the vector has been open for years. |
We'd be making it more accessible for those users that are not technically able to exploit it in the way you've described. |
@t3chguy I think the spam vector is "pending internal context" and refers to some rooms being configured so that participants can not post comments, but are still allowed to add reactji. |
Security via obscurity is bogus. Someone will write a basic guide:
|
Fixes element-hq/element-web#19409
This unleashes user creativity and allows them to react in whatever shape or form they desire. Free-form reactions don't get stored in the "Recently Used" list since they aren't emojis and I wasn't sure what that'll break – though they come as
strings
so it should be fine, but on the note of encouraging creativity perhaps it shouldn't be stored for later anyway :)This is basically My First React[tm], so let me know if something can be made less awful.
Here's what your changelog entry will look like:
✨ Features
Preview: https://616996feafffde0805ad1da9--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.