Page MenuHomePhabricator

DRAFT: [native] fix forcing message height breaking message timestamp
AbandonedPublic

Authored by ginsu on Aug 21 2023, 10:18 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Jan 8, 5:44 PM
Unknown Object (File)
Mon, Jan 6, 5:27 PM
Unknown Object (File)
Fri, Jan 3, 5:11 PM
Unknown Object (File)
Sun, Dec 29, 11:53 AM
Unknown Object (File)
Dec 4 2024, 1:28 AM
Unknown Object (File)
Nov 26 2024, 11:11 AM
Unknown Object (File)
Nov 23 2024, 1:47 AM
Unknown Object (File)
Nov 21 2024, 7:26 AM
Subscribers

Details

Reviewers
None
Summary

This is a draft of an alternate "quick and dirty solution" where we revert back to the solution where we measure just the inner message and the inline engagement. The issue with this solution was that the RetrySend component was not visible because I was initially not considering this element when we were forcing the height for composed messages.

However, we don't need to consider this element when we are forcing the height. We only need to force the height of the message that is being measured (which is the inner message and the inline engagement). So I believe an alternate solution that could work here is we can place the RetrySend component outside of the View with the viewStyle (see inline comment below for more clarity) so we aren't considering RetrySend in the first place when we force the height.

Placing RetrySend outside this View should be find since in theory an InlineEngagement and RetrySend should never be rendered at the same time since you can't edit, react, or create a sidebar on a failed local message.

I am aware that with this solution we are losing the benefits of reducing the amount of repeated code and localizing the height-warning logic in the same place that the height-forcing logic is in; however, some of the other solutions that I am exploring are a little more complicated/I need to spend a bit more time testing than I initially expected and I want to get something out soon to unblock native releases asap.

Test Plan

Since this is a draft I didn't create any demo videos, but if we do decide to go with this solution than I will create those. However, still tested the follwing:

Tested these flows and confirmed the following with forcing the height measurement:

  • Rendered the composed messages (text/multimedia) and the messages did not shift like they do when I force an incorrect height
  • Rendered the robotext messages and the messages did not shift like they do when I force an incorrect height
  • Tested adding/removing reactions
  • Tested adding sidebar
  • Tested editing
  • Tested sending a failed message and confirmed that RetrySend was visible
  • Tapped on the message and saw that the timestamp is visible
  • Tested reply message and everything worked like before
  • Tested copy message and everything worked like before
  • Tested horizontal message gesture (reply/sidebar) and everything worked like before

Diff Detail

Repository
rCOMM Comm
Branch
eng-4672 (branched from master)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ginsu retitled this revision from [native] fix forcing message height breaking message timestamp to DRAFT: [native] fix forcing message height breaking message timestamp.Aug 21 2023, 10:37 PM
ginsu edited the summary of this revision. (Show Details)
ginsu edited the test plan for this revision. (Show Details)
ginsu added reviewers: ashoat, tomek.
ginsu removed reviewers: ashoat, tomek.

Removed reviewers since this is a "draft" diff

ginsu added inline comments.
native/chat/composed-message.react.js
239

Moved failedSendInfo outside from where we force the height

D8921 makes this diff obsolete