Page MenuHomePhabricator

[native] introduced reactions to chat item height measurer
ClosedPublic

Authored by ginsu on Dec 5 2022, 5:20 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 24, 8:27 AM
Unknown Object (File)
Sun, Nov 24, 8:05 AM
Unknown Object (File)
Sun, Nov 24, 7:45 AM
Unknown Object (File)
Sun, Nov 24, 5:41 AM
Unknown Object (File)
Thu, Nov 7, 5:45 AM
Unknown Object (File)
Thu, Nov 7, 5:45 AM
Unknown Object (File)
Wed, Nov 6, 6:20 AM
Unknown Object (File)
Mon, Nov 4, 10:01 PM
Subscribers

Details

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 requested review of this revision.Dec 5 2022, 5:32 PM
ginsu edited the test plan for this revision. (Show Details)
ginsu added reviewers: atul, tomek, rohan.

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

atul added a reviewer: ashoat. atul removed 1 blocking reviewer(s): atul.

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

ashoat requested changes to this revision.Dec 8 2022, 8:55 AM
  1. 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?
  2. 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
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

  1. 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?
  2. 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 diff is now combined with D5815, because I think it should address most of the feedback/questions from @tomek/@ashoat

But to answer the first question, this diff is getting ChatItemHeightMeasurer to measure and account for reactions in InlineSidebar.

tomek added inline comments.
native/types/chat-types.js
25

$ReadOnlyMap

This revision is now accepted and ready to land.Dec 13 2022, 4:22 PM