Details

Reviewers

ashoat
atul

Summary

Rendered user avatars in the chat screen. Avatars only render for users who are not the viewer and at the end of a message cluster. When the message gets swiped, the avatar will move with the message.

UPDATE: First draft of diff did not consider the width of chat bubbles being affected by the new avatar component. I changed the value of some composedMessageMaxWidth instances to consider avatarOffset

Depends on D7099

Linear Task: https://linear.app/comm/issue/ENG-3107/build-out-a-dummy-avatar-component-and-render-it-everywhere-we-will

Test Plan

Please watch the demo videos to see all the cases I tested

UPDATE: after changing the value of some composedMessageMaxWidth instances to consider avatarOffset, the message boxes now have the correct width, and I also double-checked to make sure I did not have incorrect height measurements for the messages

Before:

Screenshot 2023-03-15 at 2.13.21 PM.png (1×1 px, 1 MB)

After:

Screenshot 2023-03-15 at 4.06.49 PM.png (1×1 px, 1 MB)

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ginsu created this revision.Tue, Mar 14, 1:37 PM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptTue, Mar 14, 1:37 PM

ginsu edited the summary of this revision. (Show Details)Tue, Mar 14, 1:41 PM

ginsu edited the test plan for this revision. (Show Details)

ginsu added reviewers: ashoat, atul.

ginsu added a parent revision: D7057: [native] render avatar in profile screen.

ginsu edited the summary of this revision. (Show Details)

ginsu attached a referenced file: F427550: Screen Recording 2023-03-14 at 4.33.07 PM.mov. (Show Details)

ginsu attached a referenced file: F427552: Screen Recording 2023-03-14 at 4.33.50 PM.mov. (Show Details)

ginsu retitled this revision from [native] render avatars in chat screen to [native] render user avatars in chat screen.

ginsu edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B17329: Diff 23735.Tue, Mar 14, 1:52 PM

ginsu requested review of this revision.Tue, Mar 14, 1:52 PM

ginsu added a child revision: D7069: [native] render the rest of the user avatars.Tue, Mar 14, 2:14 PM

Will this potentially affect the width of the chat bubbles? Have you made sure that that the height measurement code is still working correctly? (Are any seeing any of these errors?)

Have you made sure that that the height measurement code is still working correctly?

I double-checked this and everything seems fine + not getting any of these errors

Will this potentially affect the width of the chat bubbles?

Ah after a second take I did seem to miss this:

Should I decrease the width of the inner message (the chat bubble that is sticking out), or should we increase the width of the message box of the composed message (currently has the red border around it)? I personally think there is still enough space on the right side to just increase the width of the message box, but if we think it would be better to decrease the width of the inner message to fit inside the message box lmk

ginsu attached a referenced file: F428518: Screenshot 2023-03-15 at 2.13.21 PM.png. (Show Details)Wed, Mar 15, 11:36 AM

Should I decrease the width of the inner message (the chat bubble that is sticking out), or should we increase the width of the message box of the composed message (currently has the red border around it)? I personally think there is still enough space on the right side to just increase the width of the message box, but if we think it would be better to decrease the width of the inner message to fit inside the message box lmk

Defer to you and Ted! I was just asking about the width because it might affect the height calculation.

With the red border, is that a parent component of the message box? (Generally I would expect the parent component to enclose the child component.)

With the red border, is that a parent component of the message box? (Generally I would expect the parent component to enclose the child component.)

Yes the component with red border it is a parent component of InnerTextMessage (which is the chat bubble poking out)

Hmm, not sure if that's an issue or not. I think it might be best to try using things like paddingLeft / marginLeft instead of left and position: 'absolute'... worried that border miscalculations can lead to issues, but not immediately sure what sort of concrete issues

fix maxWidth for chat bubble so it's not poking out anymore (see test plan for visulization)

ginsu edited the summary of this revision. (Show Details)Wed, Mar 15, 2:26 PM

ginsu edited the test plan for this revision. (Show Details)

ginsu attached a referenced file: F428736: Screenshot 2023-03-15 at 2.13.21 PM.png. (Show Details)

ginsu attached a referenced file: F428737: Screenshot 2023-03-15 at 4.06.49 PM.png. (Show Details)

ginsu edited the summary of this revision. (Show Details)

ginsu added inline comments.

native/chat/chat-item-height-measurer.react.js
58 ↗	(On Diff #23772)	We still want the original value of useComposedMessageMaxWidth in `composed-message.react.js` so we need to do this extra step in the `composedMessageMaxWidth` calculation outside the hook

Harbormaster completed remote builds in B17355: Diff 23772.Wed, Mar 15, 2:35 PM

ashoat added inline comments.Wed, Mar 15, 4:18 PM

native/chat/chat-item-height-measurer.react.js
58 ↗	(On Diff #23772)	Is it intentional that you're reducing the width of viewer-composed messages too, even though they don't have an avatar?

ginsu planned changes to this revision.Wed, Mar 15, 5:01 PM

ginsu added inline comments.

native/chat/chat-item-height-measurer.react.js
58 ↗	(On Diff #23772)	No it isn't and I will fix that. However, this does bring up a question, I would love some input on. For viewer-composed multimedia messages, if we go back to the original width, the multimedia message is going to be a different size than non-viewer composed multimedia messages I feel like we should still reduce the width for viewer-composed multimedia messages so all the multimedia messages have a consistently smaller size, but I will bring back the original width for viewer-composed text messages. Let me know what we think

ginsu attached a referenced file: F428871: Screenshot 2023-03-15 at 7.38.28 PM.png. (Show Details)Wed, Mar 15, 5:01 PM

I feel like we should still reduce the width for viewer-composed multimedia messages so all the multimedia messages have a consistently smaller size,

Thinking about this some more, I feel that this is the best option, I was going through some chats on Comm and I feel like some of the multimedia messages are taking a bunch of screen real estate

336223399_740379237747150_8312429542900821292_n.jpg (1×828 px, 87 KB)

In contrast, FB messenger multimedia messages are much smaller:

336353513_590716002715617_5037247747435841645_n.jpg (1×828 px, 78 KB)

336162309_908146963557437_5514534208953352892_n.jpg (1×828 px, 119 KB)

ginsu attached a referenced file: F428879: 336162309_908146963557437_5514534208953352892_n.jpg. (Show Details)Wed, Mar 15, 5:16 PM

ginsu attached a referenced file: F428877: 336353513_590716002715617_5037247747435841645_n.jpg. (Show Details)

ginsu attached a referenced file: F428875: 336223399_740379237747150_8312429542900821292_n.jpg. (Show Details)

Open to decreasing media width

native/chat/chat-item-height-measurer.react.js
58 ↗	(On Diff #23772)	Yeah that's fair, it should probably be consistent. We should probably reduce the width on both sides to accomodate for the avatars on the left. Can you ask Ted about this tomorrow?

ginsu added inline comments.Wed, Mar 15, 5:25 PM

native/chat/chat-item-height-measurer.react.js
58 ↗	(On Diff #23772)	Yea for sure

Open to decreasing media width

Spoke to ted and he is for decreasing media width as well

only reduce the width of non viewer-composed messages as well as all multimedia messages

ginsu edited the summary of this revision. (Show Details)Thu, Mar 16, 2:01 PM

ginsu edited the test plan for this revision. (Show Details)

ginsu attached a referenced file: F429916: Screenshot 2023-03-16 at 4.50.46 PM.png. (Show Details)

ginsu attached a referenced file: F429918: Screenshot 2023-03-16 at 4.50.55 PM.png. (Show Details)

ginsu attached a referenced file: F429919: Screenshot 2023-03-16 at 4.51.18 PM.png. (Show Details)

Harbormaster completed remote builds in B17368: Diff 23794.Thu, Mar 16, 2:10 PM

Offline asked @ginsu if we should maintain consistency on the width of chat message bubbles for viewer vs. non-viewer, he's going to check with Ted

Offline asked @ginsu if we should maintain consistency on the width of chat message bubbles for viewer vs. non-viewer, he's going to check with Ted

I spoke to Ted about this before he headed out, and he agrees that we should maintain consistency on the width of chat message bubbles for viewer vs. non-viewer. Will have an update out here shortly; apologies for the misunderstanding above

Reduce the width for ALL composed messages bubbles to maintain consistency on the width

make width consistent for all composed messages

ginsu edited the summary of this revision. (Show Details)Thu, Mar 16, 3:21 PM

ginsu edited the test plan for this revision. (Show Details)

ginsu added inline comments.

native/chat/chat-item-height-measurer.react.js
58	We still want the original value of useComposedMessageMaxWidth in composed-message.react.js so we need to do this extra step in the composedMessageMaxWidth calculation outside the hook

Harbormaster completed remote builds in B17369: Diff 23802.Thu, Mar 16, 3:34 PM

Did you decrease both chat bubbles' sizes by avatarOffset? That means we're subtracting 2 * avatarOffset when we only need space for avatarOffset, right? Can you please adjust it appropriately to maintain previous proportions?

This revision now requires changes to proceed.Thu, Mar 16, 3:38 PM

In D7067#210294, @ashoat wrote:

Did you decrease both chat bubbles' sizes by avatarOffset? That means we're subtracting 2 * avatarOffset when we only need space for avatarOffset, right? Can you please adjust it appropriately to maintain previous proportions?

Hey @ashoat I am a bit confused by this comment, and want to make sure we are on the same page about what I need to do here. When you mean decrease both chat bubbles' sizes you are referring to both viewer and non-viewer chat bubbles correct? If you are then the answer is yes. I was under the impression that we wanted to reduce the width for all composable messages (text and multimedia) for both viewers and non-viewers to maintain a consistent width for all the chat bubbles.

Can you please adjust it appropriately to maintain previous proportions?

I am not sure what previous proportions you are referring to. Is it the previous proportions of viewer-composed text messages?

I'm thinking basically:

In current master, figure out what % of total screen width a chat bubble can take
Subtract the space necessary for the avatar from the total screen width
Keep the % from 1 the same after adjusting for 2

Does that make sense?

In D7067#210469, @ashoat wrote:

I'm thinking basically:

In current master, figure out what % of total screen width a chat bubble can take

Subtract the space necessary for the avatar from the total screen width

Keep the % from 1 the same after adjusting for 2

Does that make sense?

Okay I think I am following now. I will double-check my understanding with you IRL after lunch just so we don't waste more code review cycles

ginsu removed a child revision: D7069: [native] render the rest of the user avatars.Mon, Mar 20, 7:15 AM

rebase after changing up diff stack order

ginsu edited the summary of this revision. (Show Details)Mon, Mar 20, 7:17 AM

ginsu added a parent revision: D7099: [native] add feature flag to avatars render work.

ginsu removed a parent revision: D7057: [native] render avatar in profile screen.

Changed the stack order so that this diff comes after D7099, so that we aren't blocked on this diff for landing the avatars work once D7099 (feature flag work) is accepted. I tested on my local stack each diff when changing the order and nothing was broken

Harbormaster completed remote builds in B17410: Diff 23858.Mon, Mar 20, 7:32 AM

Still need to fix chat bubble poking out (will timebox for 1-2hrs)

[native] render user avatars in chat screen
Changes PlannedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 23802

native/chat/chat-constants.js

native/chat/chat-item-height-measurer.react.js

native/chat/composed-message.react.js

native/chat/inner-text-message.react.js

native/chat/message-header.react.js

native/chat/multimedia-message-tooltip-button.react.js

native/chat/text-message-tooltip-button.react.js

[native] render user avatars in chat screenChanges PlannedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 23802

native/chat/chat-constants.js

native/chat/chat-item-height-measurer.react.js

native/chat/composed-message.react.js

native/chat/inner-text-message.react.js

native/chat/message-header.react.js

native/chat/multimedia-message-tooltip-button.react.js

native/chat/text-message-tooltip-button.react.js

[native] render user avatars in chat screen
Changes PlannedPublic
Actions

Revision Contents
Changeset List