Details

Reviewers

michal
inka
ginsu

Commits

rCOMM0c8dcd2cac86: [web] Added displaying 'Labels' to InlineEngagment

Summary

Added the possibility to label the messages to display 'Edited' in the next diffs.

Test Plan

Run the app.
Added the label to every message.
Checked if the labels are displayed correctly both with text messages and with images.
Checked if Reactions/Threads are also correctly positioned next to the label.
Checked if labels in a few messages in a row do not take too much space.
Checked displaying on both the left and right side of the chat.

Screenshots:

Screenshot 2023-03-09 at 12.04.31.png (1×1 px, 103 KB)

Screenshot 2023-03-09 at 12.36.33.png (1×1 px, 154 KB)

Screenshot 2023-03-09 at 12.06.23.png (1×1 px, 85 KB)

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• kuba created this revision.Mar 9 2023, 3:29 AM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptMar 9 2023, 3:29 AM

• kuba added a parent revision: D6966: [lib] Add 'hasBeenEdited' status to messages.Mar 9 2023, 3:30 AM

• kuba added inline comments.

web/chat/inline-engagement.css
87 ↗	(On Diff #23573)	It's used when we only display the label, without reactions/thread icons. Did it this way in order to save space between messages without any reactions/threads.

• kuba edited the test plan for this revision. (Show Details)Mar 9 2023, 3:36 AM

Harbormaster returned this revision to the author for changes because remote builds failed.Mar 9 2023, 3:38 AM

Harbormaster failed remote builds in B17209: Diff 23573!

• kuba added a child revision: D7018: [web] Displaying 'Edit' label next to edited messages.Mar 9 2023, 3:42 AM

Harbormaster completed remote builds in B17209: Diff 23573.Mar 9 2023, 3:50 AM

• kuba requested review of this revision.Mar 9 2023, 3:50 AM

Can you check what it looks like with the "Delivery failed. RETRY?" text which appears when sending a message, and attach a screenshot? Not sure if this case was considered in the designs.

I don't think we use positioning: 'center' anywhere but can you check if it looks ok?

In D7017#209165, @michal wrote:

Can you check what it looks like with the "Delivery failed. RETRY?" text which appears when sending a message, and attach a screenshot? Not sure if this case was considered in the designs.

I'm not sure if it is the case. For now, displaying both 'delivery failed' and the label ('Edited') at the same time is impossible. For editing messages, there will be another notice if it fails to deliver.

Anyway, here is the screenshoot:

Screenshot 2023-03-13 at 15.49.19.png (190×644 px, 23 KB)

I don't think we use positioning: 'center' anywhere but can you check if it looks ok?

It is used in RobotextMessage component.

@kuba can you reach out to Ted on the design team regarding this? Design above looks weird, I would at the least swap the order of the two

I'm not sure if it is the case. For now, displaying both 'delivery failed' and the label ('Edited') at the same time is impossible.

Oh yeah, that makes sense, sorry!

I'm not sure if it is the case. For now, displaying both 'delivery failed' and the label ('Edited') at the same time is impossible.

Ah, I didn't realize. Perhaps it's not so important for the design team to take a look at it, then. But it might be good for us to "align" the delivery failed with the Edited (so that when left-aligned, all their text is left-aligned in the same place, and vice versa for right-aligned)

Aligned 'Delivery failed' notice to the message label. Only for the right side, because we can't get delivery failed on the left side.

Harbormaster completed remote builds in B17314: Diff 23714.Mar 14 2023, 7:53 AM

Can you share an updated screenshot?

In D7017#209805, @ashoat wrote:

Can you share an updated screenshot?

Yeah, sure. Sorry, somehow I didn't attach it.

Screenshot 2023-03-14 at 15.30.01.png (244×590 px, 18 KB)

Thanks!! Can you send that over to Ted on the design team to make sure he approves of the changes to FailedSend? (It's worth mentioning to Ted that it's not possible for both FailedSend and the edited label to appear at the same time.)

Left a few comments inline. Should we also add a condition to the if condition in composed-message to render the inline engagement?

Also, I personally feel like the label for edited messages should be below the reactions/threads and not to the left/right of it. Could you link or send a screenshot of what the figma has?

web/chat/inline-engagement.react.js
21 ↗	(On Diff #23714)	I think this can just be this
97–102 ↗	(On Diff #23714)	We can move this outside the useMemo hook
113 ↗	(On Diff #23714)	We shouldn't use ternary conditions inside JSX https://www.notion.so/commapp/Use-ternary-conditionals-sparingly-f4ba44a10259403592a1d15440a9847e

This revision now requires changes to proceed.Mar 16 2023, 6:40 PM

Also after reading D7018 disregard my comment above about the if statement

web/chat/inline-engagement.react.js
21 ↗	(On Diff #23714)	After reading D7018 the type should be `+label: ?string` and not `+label?: string`

In D7017#210367, @ginsu wrote:

Also, I personally feel like the label for edited messages should be below the reactions/threads and not to the left/right of it. Could you link or send a screenshot of what the figma has?

Sure, here is the latest version of the designs:
https://linear.app/comm/issue/DES-38#comment-3d7826f4
Few comments above this comment there is original figma design.

Addressed @ginsu comments

web/chat/inline-engagement.react.js
21 ↗	(On Diff #23714)	I believe that the property is optional, so it should have a `label?` (we don't want to pass `null` label i.g. in the `RobotextMessage` component). Should I change it and pass `label={null}` at each place it is used?