Details

Reviewers

michal
inka
ashoat
ginsu

Summary

Added 'hasBeenEdited' variable to track if the message was edited. Will be used to displaying 'edited' label.

Test Plan

Checked if the app compiles and works. Then tested if the status is set properly by changing labels of tooltip buttons to the edited status (instead of 'Reply' i displayed edited status).

Diff Detail

Repository

rCOMM Comm

Branch

kuba/messageediting

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

kuba created this revision.Tue, Mar 7, 1:48 AM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptTue, Mar 7, 1:48 AM

kuba added a parent revision: D6965: [lib][native] Displaying edited messages from database.Tue, Mar 7, 1:48 AM

Harbormaster completed remote builds in B17139: Diff 23479.Tue, Mar 7, 2:00 AM

kuba requested review of this revision.Tue, Mar 7, 2:00 AM

ashoat added inline comments.Tue, Mar 7, 6:06 AM

lib/selectors/chat-selectors.js
332 ↗	(On Diff #23479)
403–404 ↗	(On Diff #23479)	To work with a read-only type, you can do this
lib/types/messages/text.js
32 ↗	(On Diff #23479)	Please never add a new property that isn't read-only unless you have a good reason for it When touching a type, it's worth seeing if you can easily convert the whole thing to be read-only

ashoat mentioned this in D6965: [lib][native] Displaying edited messages from database.Tue, Mar 7, 6:07 AM

Responded to comments, made hasBeenEdited property read-only

Harbormaster completed remote builds in B17181: Diff 23531.Wed, Mar 8, 3:22 AM

Can we avoid putting this in TextMessageInfo? I don't think it belongs there... and you're doing something pretty inconsistent with how we approached reactions.

Why didn't you put this in ChatMessageInfoItem? (In the future, when you are doing something different like this, please expect that your reviewers will call you out on it, and explain it ahead of time.)

This revision now requires changes to proceed.Wed, Mar 8, 7:14 AM

Moved 'hasBeenEdited' property to 'ChatMessageInfoItem' type

In D6966#207758, @ashoat wrote:

Can we avoid putting this in TextMessageInfo? I don't think it belongs there... and you're doing something pretty inconsistent with how we approached reactions.

Why didn't you put this in ChatMessageInfoItem? (In the future, when you are doing something different like this, please expect that your reviewers will call you out on it, and explain it ahead of time.)

You are right. It should be in ChatMessageInfoItem. Going to update the diff.

I wasn't sure if we want to add it to RobotextChatMessageInfoItem, but reactions are there as well, so I also added it to it.

Harbormaster completed remote builds in B17187: Diff 23538.Wed, Mar 8, 10:16 AM

Can RobotextChatMessageInfoItem be edited?

This revision now requires changes to proceed.Wed, Mar 8, 10:16 AM

Add 'hasBeenEdited' to code for native to be able to access the variable

Harbormaster completed remote builds in B17207: Diff 23567.Thu, Mar 9, 1:46 AM

Removed 'hasBeenEdited' variable from 'RobotextChatMessageInfoItem'

Harbormaster completed remote builds in B17208: Diff 23568.Thu, Mar 9, 2:06 AM

kuba added a child revision: D7017: [web] Added displaying 'Labels' to InlineEngagment.Thu, Mar 9, 3:30 AM

ashoat accepted this revision.Thu, Mar 9, 9:52 AM

This revision is now accepted and ready to land.Thu, Mar 9, 9:52 AM