Page MenuHomePhabricator

[native] introduce DummyInlineEngagementNode
ClosedPublic

Authored by ginsu on Jul 24 2023, 7:12 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 29, 2:38 PM
Unknown Object (File)
Fri, Nov 29, 1:19 PM
Unknown Object (File)
Tue, Nov 26, 3:46 AM
Unknown Object (File)
Tue, Nov 26, 1:34 AM
Unknown Object (File)
Sat, Nov 23, 6:54 PM
Unknown Object (File)
Sat, Nov 23, 4:18 PM
Unknown Object (File)
Sat, Nov 23, 4:18 PM
Unknown Object (File)
Sat, Nov 23, 4:18 PM
Subscribers

Details

Summary

This next part of the stack will introduce the logic for updating the chat item height measurer to include the inline engagement in the message measurement. The order of operations for this next part of the stack will be correctly measuring the robotext message, then a text message, then finally a multimedia message.

The first diff introduces the dummy inline engagement component that will be a part of the heightMeasurerDummy which will get passed to the NodeHeightMeasurer. Subsequent diffs will handle updating the heightMeasurerKey function.

Depends on D8609

Test Plan

Confirmed that the incorrect height measurement log was no longer in the console. Also double checked in the inspector that I was getting the correct height measurements for both dummy nodes. I will also be continually testing this component in subsequent diffs

Before:

Screenshot 2023-07-24 at 9.33.43 PM.png (1×3 px, 1 MB)

After:

Screenshot 2023-07-24 at 9.36.34 PM.png (1×3 px, 1 MB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ginsu requested review of this revision.Jul 24 2023, 7:39 PM
native/chat/inline-engagement.react.js
66–68 ↗(On Diff #28986)

I didn't include the sidebar icon here on purpose. I thought it would be better if we didn't render a CommIcon component and instead just increased the paddingLeft in dummySidebar to accommodate for the icon.

Screenshot 2023-07-24 at 10.46.20 PM.png (82×184 px, 11 KB)

native/chat/inner-robotext-message.react.js
32 ↗(On Diff #28986)

I attempted to use a react fragment here; however, it always resulted in a white box appearing on the screen for a few seconds. When I used a View, I did not encounter this issue

Screenshot 2023-07-24 at 10.56.19 PM.png (1×1 px, 1 MB)

native/chat/utils.js
94–101 ↗(On Diff #28986)

Since the inline engagement is already included in the contentHeight we don't need to do this check anymore

Adding @tomek and @ashoat as reviewers since I haven't worked with any of the height measurement stuff

Looks ok but please make sure that we avoid duplicating style props.

native/chat/inline-engagement.react.js
54 ↗(On Diff #28986)

Why do we use unboundStyles?

344–345 ↗(On Diff #28986)
410–425 ↗(On Diff #28986)

We should avoid duplicating styles as much as possible to avoid bugs caused by them getting out of sync. Instead, we can introduce new styles that override / add some props and use both, e.g.

<Text style={[unboundStyles.messageLabel, unboundStyles.dummmyMessageLabel]}>{editedLabel}</Text>
This revision is now accepted and ready to land.Jul 26 2023, 3:03 AM
native/chat/inline-engagement.react.js
54 ↗(On Diff #28986)

Since we don't really care about colors for the dummy nodes. I thought it would be a good idea to skip the const styles = useStyles(unboundStyles); step.

This follow a similar pattern used in other places where we measure nodes:

InnerTextMessage does not follow this exact pattern; however, its style sheet does not use any colors so we do not need to have an unboundStyles object:
https://github.com/CommE2E/comm/blob/master/native/chat/inner-text-message.react.js#L162-L176

410–425 ↗(On Diff #28986)

We should avoid duplicating styles as much as possible to avoid bugs caused by them getting out of sync.

This is a good point. I think the best way to go about this is to separate out the color style from the object and have it used in other object. For example:

messageLabel: {
  paddingHorizontal: 3,
  fontSize: 13,
  top: inlineEngagementLabelStyle.topOffset,
  height: inlineEngagementLabelStyle.height,
  marginTop: inlineEngagementStyle.marginTop,
},
dummyMessageLabel: {
  marginLeft: 9,
  marginRight: 4,
}
messageLabelColor: {
   color: 'messageLabel',
}

Then we can use messageLabel in both the actual node and the dummy node (which should address this concern), we still don't need to introduce useStyle hook in the DummyInlineEngagementNode component, and if we need specific styles for the dummy node we still have a style object for it.

address duplicating styles concern

native/chat/inline-engagement.react.js
141–156 ↗(On Diff #29079)

Not super related to this diff, but when I was addressing the duplicated styles concern, this seemed like a good opportunity to memoize the editedLabelStyle separately from editedLabel

fix duplicate style for inline engagement container + rebase before landing

This revision was landed with ongoing or failed builds.Jul 31 2023, 1:19 PM
This revision was automatically updated to reflect the committed changes.