Page MenuHomePhabricator

[native] fix forcing the incorrect height for multimedia messages
ClosedPublic

Authored by ginsu on Aug 12 2023, 1:30 PM.
Tags
None
Referenced Files
F3299655: D8799.diff
Sun, Nov 17, 2:30 PM
Unknown Object (File)
Tue, Oct 22, 6:37 AM
Unknown Object (File)
Tue, Oct 22, 6:37 AM
Unknown Object (File)
Tue, Oct 22, 6:37 AM
Unknown Object (File)
Tue, Oct 22, 6:35 AM
Unknown Object (File)
Tue, Oct 22, 5:03 AM
Unknown Object (File)
Oct 14 2024, 5:02 PM
Unknown Object (File)
Oct 8 2024, 3:21 PM
Subscribers

Details

Summary

This diff is part 3/3 to fix forcing the incorrect height for messages with inline engagement in prod.

This linear comment explains this issue and what I did to address this

Since item.contentHeight is now the height of the inner text message AND the height of the inline engagement we are now forcing the wrong height for the inner text message component. An idea I want to explore to address this is to force the height higher up in the component tree (somewhere like ComposedMessage) where both the "inner message" and the inline engagement is rendered so that when we force the height withitem.contentHeight, it is correct

Depends on D8798

Test Plan

Please see demo video where I force and unforce the correct height of the multimedia messages and it did not change visually (outside of the green background) and I did not get any error logs talking about incorrect height measurements, I also purposely force an incorrect height (which resulted in some logs about incorrect height measurements.

Also please note for the video, I set a green background so that it is easier to visually see what is happening, but these are only for the demo videos

With the correct height measurement:

With the incorrect height measurement:

Here is also a demo video to show that there was no regression by removing the expand style (I pressed and was able to view the the full screen image for multimedia messages with both a single image and multiple images):

Also here is another demo video to view the the full screen image for multimedia messages with both a single image and multiple images W/O an inline engagement

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ginsu requested review of this revision.Aug 12 2023, 1:49 PM
native/chat/multimedia-message.react.js
235–239 ↗(On Diff #29848)

This code was causing the inline engagement to look like this for multimedia messages:

Screenshot 2023-08-12 at 4.46.33 PM.png (1×1 px, 1 MB)

Looked at the blame history for this and found out that this was introduced in D1640. When I removed this code, I followed the test plan for this diff very closely and made sure that I did not introduce any regressions and I could still open the image modal for multimedia messages when I tapped on it.

Make sure you address the inline comment before landing

native/chat/multimedia-message.react.js
235–239 ↗(On Diff #29848)

Can you modify the Test Plan to include the testing you did to make sure you didn't regress D1640? Please explicitly call out whether you tested pressing a multimedia message with multiple images

This revision is now accepted and ready to land.Aug 15 2023, 6:31 AM
ginsu edited the test plan for this revision. (Show Details)EditedAug 15 2023, 9:35 AM

updated test plan and addressed comment

This revision was landed with ongoing or failed builds.Aug 15 2023, 9:44 AM
This revision was automatically updated to reflect the committed changes.