Details

Reviewers

atul
tomek
rohan
ashoat

Commits

rCOMM3e1cc43ba3b1: [native] introduced reactions to chat item height measurer

Summary

introduced reactions to chat item height measurer. This is necessary to because in native/chat/message-list-container we measure the messages first and then pass the message list data with heights down to MessageList which will eventually then be used by InlineSidebar

Depends on D5811
Linear Task: ENG-2408

Test Plan

Will be doing more tests in subsequent diffs. Ran mobile/web app locally and nothing crashes. I also was able to get to the point where I could send and reaction and it would render on my local stack (see demo video below)

Also, before these changes I was getting this error message in my logs telling me that the height was incorrect; however, after these changes the error message went away showing that InlineSidebar was taken into consideration

Screenshot 2022-12-05 at 9.34.38 PM.png (1×3 px, 939 KB)

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ginsu created this revision.Dec 5 2022, 5:20 PM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptDec 5 2022, 5:20 PM

Harbormaster completed remote builds in B13979: Diff 19152.Dec 5 2022, 5:32 PM

ginsu requested review of this revision.Dec 5 2022, 5:32 PM

ginsu edited the summary of this revision. (Show Details)Dec 5 2022, 5:33 PM

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

ginsu added reviewers: atul, tomek, rohan.

ginsu attached a referenced file: F272413: Screen Recording 2022-12-05 at 8.04.10 PM.mov. (Show Details)

ginsu added a child revision: D5813: [native] introduce reactions to inline sidebar.Dec 5 2022, 5:37 PM

ginsu added a child revision: D5814: [web] introduce reactions to inline sidebar.Dec 5 2022, 6:08 PM

ginsu edited the summary of this revision. (Show Details)Dec 5 2022, 7:13 PM

ginsu added a parent revision: D5811: [lib] introduced reactions field to chatMessageItems.

Note that InlineSidebar is not currently measured by ChatItemHeightMeasurer: ENG-775

I think we instead have some hardcoded value somewhere that contains its height, so we should make sure to update that as we update InlineSidebar

native/types/chat-types.js
25 ↗	(On Diff #19152)	Why optional?

tomek accepted this revision.Dec 6 2022, 6:53 AM

tomek added inline comments.

native/chat/chat-item-height-measurer.react.js
151 ↗	(On Diff #19152)	How does the measurer know how to measure a reaction?
native/types/chat-types.js
59 ↗	(On Diff #19152)	It seems quite extensive to provide the whole list of reactions while the only thing needed for now is a flag telling if there's any reaction, but maybe in the future we would have different height based on number and type of reactions.

Looks reasonable, adding @ashoat as a blocking reviewer since I don't have much context on NodeHeightMeasurer

Please respond to my question earlier. It's not clear to me what this diff is doing... is it getting ChatItemHeightMeasurer to measure reactions? Or is it just adding some parameter to fix some Flow issue?
Can you update the Test Plan to cover making sure that either:
- The measurements are correct (ie. this doesn't trigger on iOS like it does for Android on master, cc @rohan) AND the measurements take InlineSidebar into consideration (looks like they don't?)
- If incorrect measurements don't get caught (eg. the code above is not triggered and/or the measurement's don't take InlineSidebar into consideration), can you make sure that our manual height calculations are still correct?

This revision now requires changes to proceed.Dec 8 2022, 8:55 AM

addressed feedback

ginsu added inline comments.Dec 12 2022, 8:16 PM

native/types/chat-types.js
25 ↗	(On Diff #19152)	Sorry this took me a while to address. I generally have a preference for making "empty" data structures optional but spoke to @atul last week about this, and we came to the conclusion that it is better to just have them be required and empty

Harbormaster completed remote builds in B14098: Diff 19298.Dec 12 2022, 8:23 PM

In D5812#174606, @ashoat wrote:

Please respond to my question earlier. It's not clear to me what this diff is doing... is it getting ChatItemHeightMeasurer to measure reactions? Or is it just adding some parameter to fix some Flow issue?

Can you update the Test Plan to cover making sure that either:

The measurements are correct (ie. this doesn't trigger on iOS like it does for Android on master, cc @rohan) AND the measurements take InlineSidebar into consideration (looks like they don't?)

If incorrect measurements don't get caught (eg. the code above is not triggered and/or the measurement's don't take InlineSidebar into consideration), can you make sure that our manual height calculations are still correct?