Page MenuHomePhabricator

[web] Added displaying 'Labels' to InlineEngagment
ClosedPublic

Authored by kuba on Mar 9 2023, 3:29 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Feb 22, 1:00 AM
Unknown Object (File)
Wed, Feb 21, 4:08 AM
Unknown Object (File)
Wed, Feb 21, 2:48 AM
Unknown Object (File)
Wed, Feb 21, 2:48 AM
Unknown Object (File)
Wed, Feb 21, 2:48 AM
Unknown Object (File)
Wed, Feb 21, 2:48 AM
Unknown Object (File)
Wed, Feb 21, 2:48 AM
Unknown Object (File)
Wed, Feb 21, 2:48 AM
Subscribers

Details

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 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.

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!

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?

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.

Can you share an updated screenshot?

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.)

ginsu requested changes to this revision.Mar 16 2023, 6:40 PM

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)
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.

kuba marked 4 inline comments as done.

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?

Added keys to the components list to prevent key error

web/chat/inline-engagement.react.js
117 ↗(On Diff #24269)

Can we please avoid inline logic in JSX? JSX should be a "leaf node" in the AST

ginsu added inline comments.
web/chat/inline-engagement.react.js
21 ↗(On Diff #23714)

Hey sorry this took me so long to reply... I did not consider RobotextMessage, but you are right! thanks for clarifying!

This revision is now accepted and ready to land.Mar 29 2023, 4:06 PM

Matched edited label color with the designs

This revision was landed with ongoing or failed builds.Mar 30 2023, 7:37 AM
This revision was automatically updated to reflect the committed changes.