Details
This will be tested further in subsequent diffs. Ran this locally on web/mobile and nothing crashed. Also helped me get to the point where I could push a reaction message into the DB (see screenshot below)
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
working on message reducer stuff right now and I have a strong intuition that what I return in sendReactionMessage will have to change slightly
replaced time with newMessageInfo in return and created a new type to reflect these changes
| lib/actions/message-actions.js | ||
|---|---|---|
| 210 ↗ | (On Diff #19049) | When we allow setting a particular reaction, we should also allow removing just it, and not all the reactions. Setting null doesn't allow that. |
| lib/actions/message-actions.js | ||
|---|---|---|
| 210 ↗ | (On Diff #19049) | This raises a good question. I think that we've been working under the assumption that a user can only react to a message with a single reaction (eg iMessage, FB Messenger, Telegram, etc). However, apps like Slack and Discord allow a single user to have multiple reactions to a message. I think for now we're planning on only letting a user react to a message with a single reaction, but maybe we should consider designing this feature in a flexible way that allows for enabling "multiple reactions" in the future? Personally think it's fine to proceed as is, but CC @ashoat |
| lib/actions/message-actions.js | ||
|---|---|---|
| 210 ↗ | (On Diff #19049) | yea I was definitely under the impression that reactions would work similarly to iMessage/FB messenger, but if we want reactions similar to slack/discord, I'm probably going to have to rework a few things. I will talk more with Atul about this in our sync later today |
Sending back to your queue to update design as we discussed (include some sort of boolean or enum that describes whether a reaction is being set or "unset")
| lib/actions/message-actions.js | ||
|---|---|---|
| 211 ↗ | (On Diff #19119) | It's a big improvement - the API is now flexible enough to handle both actions. But I'm not sure about the name. First of all, unsetting would indicate that we're setting a reaction, but instead we're reacting, so maybe removing is more appropriate? So overall, I'm not sure about the best solution here, but also don't think that it's worth spending a lot of time on it. Maybe finding a little bit better name should be enough. |
| lib/actions/message-actions.js | ||
|---|---|---|
| 211 ↗ | (On Diff #19119) | Agree we shouldn't waste too much time on this |
Can we quickly update unsettingReaction field to action field that's an enum (eg 'add_reaction' | 'remove_reaction')
| lib/actions/message-actions.js | ||
|---|---|---|
| 211 ↗ | (On Diff #19119) | I think unsettingReaction is a weird name for this field. If we want to keep it as a boolean we could do something like isReactionBeingSet... but then it's not explicit that when false it's unsetting the reaction. I think it'd be cleaner have a action field that's an enum... something like: action: 'add_reaction' | 'remove_reaction' or action: 'add' | 'remove' or action: 'react' | 'unreact' etc |
| lib/actions/message-actions.js | ||
|---|---|---|
| 240 ↗ | (On Diff #19145) | did a deep dive into call-server-endpoint and came to the conclusion that interface is actually unnecessary for reaction message |
| lib/types/message-types.js | ||
| 529 ↗ | (On Diff #19145) | did a deep dive into call-server-endpoint and came to the conclusion that interface is actually unnecessary for reaction message |