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
No Lint Coverage - Unit
No Test Coverage
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 |
You don't need to use React.useMemo(...) here.
The useMemo() hook is helpful for maintaining referential equality so that objects will be considered "shallowly equal" (== in JS) and we can avoid re-renders. This is helpful for objects (including Map(), Set(), etc), arrays (which are objects), and functions (which are objects).
On the other hand, strings in JS are considered shallowly equal if they have the same contents, so we don't have to worry about re-renders if the "content" stays the same.
See below:
(In like C++, which you've been working w/ recently, std::strings can be allocated on the heap (unlike integers, booleans, etc) which may have been part of your reasoning that shallow equality would check reference instead of contents?)