Details

Reviewers

atul
kamil
ashoat

Commits

rCOMMdf101e5f740a: [native] fix RetrySend not visible when message fails

Summary

This diff fixes the regression where the RetrySend component doesn't appear when a message fails. This was due to me not considering this element when we were forcing the height for composed messages.

To fix this I am now forcing the height in Message and forwarding down viewStyle to TextMessage, MultimediaMessage, and RobotextMessage. This will end up setting the height in exactly the same places that it's being set in the previous revision, but now with the added benefit of localizing the height-warning logic in the same place that the height-forcing logic. We also actually already have a function (messageItemHeight) that calculates the height of this root view for all three of the message types so we can just reuse that here.

Test Plan

Please see demo video where I force and unforce the height of the composed message and the RetrySend component remains visible. I also confirmed that I did not get any error logs talking about incorrect height measurement with this change for any of the three message types.

Before:

After (the green background is just to show that the simulator is being updated when forcing/unforcing the height):

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ginsu created this revision.Aug 16 2023, 4:43 PM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptAug 16 2023, 4:43 PM

ginsu edited the test plan for this revision. (Show Details)Aug 16 2023, 4:54 PM

ginsu added reviewers: atul, kamil, ashoat.

ginsu attached a referenced file: F692071: Screen Recording 2023-08-16 at 7.49.32 PM.mov. (Show Details)

ginsu attached a referenced file: F692062: Screen Recording 2023-08-16 at 7.48.55 PM.mov. (Show Details)

Harbormaster completed remote builds in B21812: Diff 29983.Aug 16 2023, 5:00 PM

ginsu requested review of this revision.Aug 16 2023, 5:00 PM

ginsu added inline comments.Aug 16 2023, 5:01 PM

native/chat/composed-message.react.js
232–239 ↗	(On Diff #29983)	Should have paid more attention to the children of this `View` when fixing this the first time around... Clearly `failedSendInfo` is going to affect the height here. I believe now that we consider `failedSendInfo` we should not see anymore regressions when it comes to forcing the height of this part of the composed message since `contentHeight` considers the first child `View` (the message box), `failedSendInfo` is now considered in the height measurement, and `inlineEngagement` is considered in `contentHeight` for text messages and `inlineEngagementHeight` for multimedia messages

Where did we factor in failedSendHeight in the code before your changes?

Where did we factor in failedSendHeight in the code before your changes?

https://github.com/CommE2E/comm/blob/master/native/chat/utils.js#L57

https://github.com/CommE2E/comm/blob/master/native/chat/multimedia-message-utils.js#L104

On second thought, instead of forcing the height on line 232, it might be better to force the height on line 227. If we do this, I think we should be able to just easily reuse these methods above entirely instead of partially duplicating this logic in lines 213-234. (still need to confirm this)

Removing this from reviewers queue until I confirm my comment above

simplify height forcing logic

ginsu edited the summary of this revision. (Show Details)Aug 17 2023, 12:52 AM

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

ginsu attached a referenced file: F692535: Screen Recording 2023-08-17 at 3.50.32 AM.mov. (Show Details)

ginsu edited the test plan for this revision. (Show Details)Aug 17 2023, 12:54 AM

ginsu attached a referenced file: F692546: Screen Recording 2023-08-17 at 3.52.53 AM.mov. (Show Details)

Harbormaster completed remote builds in B21821: Diff 29993.Aug 17 2023, 12:57 AM

ginsu edited the test plan for this revision. (Show Details)Aug 17 2023, 1:22 AM

ginsu attached a referenced file: F692597: Screen Recording 2023-08-17 at 4.19.42 AM.mov. (Show Details)

ginsu added inline comments.Aug 17 2023, 1:31 AM

native/chat/failed-send.react.js
142 ↗	(On Diff #29993)	Having `flex: 1` here would cause the `failedSendInfo` not to render when forcing the height. We still get the correct height, but we just can't see `failedSendInfo` Please see the after demo video above to see that we can see `failedSendInfo` when we remove this style

ginsu attached a referenced file: F692652: Screenshot 2023-08-17 at 4.28.17 AM.png. (Show Details)Aug 17 2023, 1:31 AM

use messageItemHeight

native/chat/failed-send.react.js
142 ↗	(On Diff #29995)	Having flex: 1 here would cause the failedSendInfo not to render when forcing the height. We still get the correct height, but we just can't see failedSendInfo Please see the after demo video above to see that we can see failedSendInfo when we remove this style

ginsu attached a referenced file: F692698: Screenshot 2023-08-17 at 4.28.17 AM.png. (Show Details)Aug 17 2023, 1:37 AM

ginsu edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B21823: Diff 29995.Aug 17 2023, 1:54 AM

Based on how viewProps appears to be forwarded, I think it would be possible to set viewStyle in Message, and pass it as a style prop to TextMessage, MultimediaMessage, and RobotextMessage. The former two would then forward it to ComposedMessage, and the style prop would end up being set in exactly the same places that it's being set in this diff today.

Besides reducing the amount of repeated code, this has the benefit of localizing the height-warning logic in the same place that the height-forcing logic is in, which would allow us to simplify this comment:

// We don't force view height in dev mode because we
// want to measure it in Message to see if it's correct

This revision now requires changes to proceed.Aug 17 2023, 6:42 AM

address comments

ginsu edited the summary of this revision. (Show Details)Aug 17 2023, 10:01 AM

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

ginsu attached a referenced file: F693639: Screen Recording 2023-08-17 at 12.56.13 PM.mov. (Show Details)

ginsu edited the summary of this revision. (Show Details)Aug 17 2023, 10:03 AM

ginsu added inline comments.

native/chat/failed-send.react.js
142 ↗	(On Diff #30021)	Having `flex: 1` here would cause the `failedSendInfo` not to render when forcing the height. We still get the correct height, but we just can't see `failedSendInfo` Please see the after demo video above to see that we can see `failedSendInfo` when we remove this style

ginsu attached a referenced file: F693681: Screenshot 2023-08-17 at 4.28.17 AM.png. (Show Details)Aug 17 2023, 10:05 AM

Harbormaster completed remote builds in B21848: Diff 30021.Aug 17 2023, 10:18 AM

Let's avoid forcing each of the TextMessage, RobotextMessage, and MultimediaMessage components from rerendering on every render of Message. We should memoize the style object we pass in

native/chat/message.react.js

67–72 ↗

(On Diff #30021)

// We don't force view height in dev mode because we
// want to measure it in the onLayout below to see if it's correct
const viewStyle = __DEV__ ? undefined : this.props.viewStyle;

130 ↗

(On Diff #30021)

Replace this with this.props.viewStyle.height

158 ↗

(On Diff #30021)

const viewStyle = React.useMemo(() => ({
  height: messageItemHeight(props.item),
}), [props.item]);

This revision is now accepted and ready to land.Aug 17 2023, 10:55 AM

memoize viewStyle

ginsu added inline comments.Aug 17 2023, 4:11 PM

native/chat/message.react.js
49 ↗	(On Diff #30048)	I created this type here since the `ViewStyle` type does not guarantee that `height` will be set. I feel like there is a better way to annotate this and will spend a bit more time trying to figure this out, but just wanted to get the diff updated in case anyone has an idea off the top of their head cc @ashoat