Details

Reviewers

ashoat
atul

Commits

rCOMM6f269a0c7bd9: [native] render user avatars in chat screen

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

multimedia message with multiple images:

video message:

Chat screen pre avatar world (when !shouldRenderAvatars)

Chat screen post avatar world (when shouldRenderAvatars)

Under the hood here are what these components look like:
Chat screen pre avatar world:

Screenshot 2023-03-23 at 12.44.26 PM.png (1×1 px, 1 MB)

Chat screen post avatar world:

Screenshot 2023-03-23 at 12.44.09 PM.png (1×1 px, 735 KB)

Super thin photo:
pre avatars world:

post avatars world:

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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

ginsu edited the summary of this revision. (Show Details)Mar 16 2023, 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.Mar 16 2023, 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)Mar 16 2023, 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 ↗	(On Diff #23802)	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.Mar 16 2023, 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.Mar 16 2023, 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.Mar 20 2023, 7:15 AM

rebase after changing up diff stack order

ginsu edited the summary of this revision. (Show Details)Mar 20 2023, 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.Mar 20 2023, 7:32 AM

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

add feature flags and fix poking chat bubble

My last proposed solution to fix the chat bubble poking out of the message box is to make the message box a flex container and align the inner text messages (either flex-end or flex-start) depending on if the message came from the viewer or not. This solution is very similar to how we position the InlineEngagement component, which inspired me to implement the solution this way.

So under the hood this is what the chat bubble (green) and message box (red) look like:

Screenshot 2023-03-21 at 5.31.04 PM.png (1×1 px, 1 MB)

The message box is now the size of the available real estate (screen width minus a horizontal side margin). The inner text message (chat bubble) will either move to the left of the message box (using flex-start) or the right of it (using flex-end) just like how InlineEngagement behaves.

When testing all of this, I am still maintaining the previous proportions as before I rendered the avatar screen and the swiping still works great:

I am also not getting any logs regarding incorrect height measurements.

I feel pretty good that this is the best solution; however, if this is not the case, I will need your help @ashoat to figure this out quickly

ginsu attached a referenced file: F441058: Screenshot 2023-03-21 at 5.31.04 PM.png. (Show Details)Mar 22 2023, 3:43 PM

ginsu attached a referenced file: F441060: Screen Recording 2023-03-22 at 6.28.52 PM.mov. (Show Details)

Harbormaster completed remote builds in B17535: Diff 24006.Mar 22 2023, 3:57 PM

That general approach looks great!! Great work figuring this out.

Just minor comments at this point. Can you amend your Test Plan to:

Include testing a multimedia with multiple images
Include testing a video (not sure if you already did that)
Test how everything looks if !shouldRenderAvatars

native/chat/composed-message.react.js
96 ↗	(On Diff #24006)	What happens if we always include this? Does it break rendering in the pre-avatars world?
98 ↗	(On Diff #24006)	It's pretty confusing that `styles.messageBox` is passed to `messageBoxStyleContainerStyle` rather than to `messageBoxStyle`. Can you clean up the naming? (The easiest thing would probably be to rename `styles.messageBox`)
100 ↗	(On Diff #24006)	What happens if we just add `flex: 1` to `styles.messageBox`? Does it break rendering in the pre-avatars world? In general we should move all "static" styles to `styles` rather than defining them inline, so if we really need this it would be good to do that.
272 ↗	(On Diff #24006)	What does this line do?
272 ↗	(On Diff #24006)	Do we need `display: 'flex'` here, or does React Native "assume" that by default or something?
native/chat/text-message-tooltip-button.react.js
156–171 ↗	(On Diff #24006)	Looks like you're copy-pasting this code in multiple places. When you find yourself copy-pasting something, that should be a "red flag" that you're doing something wrong. Can you factor this code out into its own component, and make sure you use that component in all of the places that you're doing this? (All places in this diff, and all places in other diffs)

This revision now requires changes to proceed.Mar 22 2023, 4:09 PM

ginsu added inline comments.Mar 23 2023, 8:38 AM

native/chat/composed-message.react.js
96 ↗	(On Diff #24006)	At one point as I was implementing this new solution the rendering broke; however, after trying to repro it to get a screenshot, everything seems to be copacetic now both in the pre and post avatar world. I think I had a style out of place as I was tinkering, which caused my misassumption. I will fix this and show this in the test plan
100 ↗	(On Diff #24006)	At one point as I was implementing this new solution the rendering broke; however, after trying to repro it to get a screenshot, everything seems to be copacetic now both in the pre and post avatar world. I think I had a style out of place as I was tinkering, which caused my misassumption. I will fix this and show this in the test plan
272 ↗	(On Diff #24006)	In RN everything is `display: 'flex'` by default https://blog.logrocket.com/using-flexbox-react-native/#:~:text=Flexbox%20is%20the%20default%20in%20React%20Native%2C%20but%20we%20don%E2%80%99t%20have%20to%20opt%20in%20to%20it%2C%20meaning%20we%20don%E2%80%99t%20need%20to%20set%20display%3A%20flex%20in%20a%20style%3A
272 ↗	(On Diff #24006)	It brings the avatar down to the bottom of the message box. Without it our avatar would be rendered like this
native/chat/text-message-tooltip-button.react.js
156–171 ↗	(On Diff #24006)	Gotcha will put up a diff to address factoring out `shouldRenderAvatars` logic into a `AvatarFeatureFlagWrapper` component

ginsu attached a referenced file: F441194: Screenshot 2023-03-22 at 7.44.33 PM.png. (Show Details)Mar 23 2023, 8:38 AM

ginsu added inline comments.Mar 23 2023, 9:12 AM

native/chat/text-message-tooltip-button.react.js
156–171 ↗	(On Diff #24006)	Spoke to @ashoat IRL and we will just be factoring out this part of the code in only `text-message-tooltip-button` and `multimedia-message-tooltip-button`

address comments

ginsu edited the test plan for this revision. (Show Details)Mar 23 2023, 9:45 AM

ginsu attached a referenced file: F441949: Screen Recording 2023-03-23 at 12.34.55 PM.mov. (Show Details)

ginsu attached a referenced file: F441950: Screen Recording 2023-03-23 at 12.35.10 PM.mov. (Show Details)

ginsu attached a referenced file: F441951: Screen Recording 2023-03-23 at 12.35.42 PM.mov. (Show Details)

ginsu attached a referenced file: F441956: Screen Recording 2023-03-23 at 12.42.26 PM.mov. (Show Details)

ginsu attached a referenced file: F441960: Screenshot 2023-03-23 at 12.44.26 PM.png. (Show Details)

ginsu attached a referenced file: F441961: Screenshot 2023-03-23 at 12.44.09 PM.png. (Show Details)

ginsu added inline comments.Mar 23 2023, 9:51 AM

native/chat/text-message-tooltip-button.react.js
162	This component and `multimedia-message-tooltip-button.react.js` and `robotext-message-tooltip-button.react.js` have a ton of code that is copy and pasted with each other (reactions, inline engagement, and some styles). This is something we shouldn't spend time on now but wonder if eventually, we should make a general `MessageTooltipButtonWrapper` component that factors out all the shared code between these three components. If this is something we should do, I can create a task to track it

Harbormaster completed remote builds in B17555: Diff 24032.Mar 23 2023, 9:52 AM

memoize MessageTooltipButtonAvatar

Harbormaster completed remote builds in B17556: Diff 24033.Mar 23 2023, 10:15 AM

ashoat accepted this revision.Mar 23 2023, 7:32 PM

ashoat added inline comments.

native/chat/text-message-tooltip-button.react.js
162	Yes, great idea for a Backlog task!!

This revision is now accepted and ready to land.Mar 23 2023, 7:32 PM

One last thing that would be great if you could test: what happens if you upload a single very "thin" photo? I want to make sure it doesn't expand to take the whole width and then becomes super tall. (Or at least, that there's no regression in this.)

Sorry for all the specific test plan requests, I just remember there being a lot of edge cases with this code from back when I was working on it.

ginsu added inline comments.Mar 23 2023, 11:44 PM

native/chat/text-message-tooltip-button.react.js
162	https://linear.app/comm/issue/ENG-2911/introduce-a-wrapper-component-for-all-the-message-tooltip-button

In D7067#212617, @ashoat wrote:

One last thing that would be great if you could test: what happens if you upload a single very "thin" photo? I want to make sure it doesn't expand to take the whole width and then becomes super tall. (Or at least, that there's no regression in this.)

Sorry for all the specific test plan requests, I just remember there being a lot of edge cases with this code from back when I was working on it.

no worries, I amended the test plan to test that case, and everything still looks great, even with a single very "thin" photo

Closed by commit rCOMM6f269a0c7bd9: [native] render user avatars in chat screen (authored by ginsu). · Explain WhyMar 23 2023, 11:51 PM

This revision was automatically updated to reflect the committed changes.

ginsu added a commit: rCOMM6f269a0c7bd9: [native] render user avatars in chat screen.

ginsu mentioned this in D7195: [web] render user avatars in chat screen.Mar 27 2023, 2:02 PM

ginsu mentioned this in rCOMM9fcb517e86f4: [web] render user avatars in chat screen.Apr 4 2023, 11:53 AM

ashoat mentioned this in D7696: [native] Clean up ComposedMessage.May 1 2023, 1:05 PM

ashoat mentioned this in rCOMMe1ea3b56bb73: [native] Clean up ComposedMessage.May 1 2023, 1:54 PM