Page MenuHomePhabricator

[native] fix broken messaging experience from forcing incorrect message height measurements
ClosedPublic

Authored by ginsu on Aug 22 2023, 12:19 PM.
Tags
None
Referenced Files
F3681027: D8921.diff
Mon, Jan 6, 5:27 PM
Unknown Object (File)
Fri, Jan 3, 2:34 AM
Unknown Object (File)
Sun, Dec 29, 11:53 AM
Unknown Object (File)
Nov 21 2024, 7:26 AM
Unknown Object (File)
Nov 21 2024, 7:26 AM
Unknown Object (File)
Nov 21 2024, 7:26 AM
Unknown Object (File)
Nov 21 2024, 7:23 AM
Unknown Object (File)
Nov 19 2024, 11:02 PM
Subscribers

Details

Summary

A quick and dirty approach to fixing forcing the incorrect height measurements 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.

Here is the Linear exchange where @ashoat and I discuss moving forward with this approach

Depends on D8920

Test Plan

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

Please note that the strange visual behavior with replying and editing messages was already here prior to any changes with the new inline engagement. I figured these issues are outside the scope of the inline engagement project and they should get addressed soon when we build out the new message action bottomsheet/tooltip

EDIT: whoops realized I didn't show Tested sending a failed message and confirmed that RetrySend was visible. Here it is:

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ginsu edited the test plan for this revision. (Show Details)
ginsu added reviewers: ashoat, tomek, atul.
This revision is now accepted and ready to land.Aug 22 2023, 9:17 PM