Page MenuHomePhabricator

[native] seperate out each reaction into their own pill
ClosedPublic

Authored by ginsu on Jul 18 2023, 2:38 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 23, 12:51 PM
Unknown Object (File)
Sat, Nov 23, 12:51 PM
Unknown Object (File)
Sat, Nov 23, 12:51 PM
Unknown Object (File)
Sat, Nov 23, 12:51 PM
Unknown Object (File)
Wed, Nov 13, 10:48 AM
Unknown Object (File)
Oct 28 2024, 7:43 AM
Unknown Object (File)
Oct 6 2024, 5:38 AM
Unknown Object (File)
Oct 6 2024, 3:21 AM
Subscribers

Details

Summary

The next step to redesigning the InlineEngagement component is to split out each reaction into its own pill. In this diff we introduced a regression where we got rid of the spacing between the sidebar pill and the first reaction pill; however, we will address this regression in the very next diff

As mentioned in a previous diff, this diff breaks the chat height measurement. As such this diff and subsequent diffs won't be landed until the rest of the native inline engagement work w/ correct height measurement is completed

Depends on D8497

Test Plan

Please see the screenshots below

Before:

Screenshot 2023-07-18 at 1.08.24 AM.png (1×960 px, 509 KB)

Screenshot 2023-07-18 at 1.05.52 AM.png (1×960 px, 423 KB)

After:

Screenshot 2023-07-18 at 1.07.37 AM.png (1×960 px, 743 KB)

Screenshot 2023-07-18 at 1.07.48 AM.png (1×1 px, 854 KB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

native/chat/inline-engagement.react.js
48 ↗(On Diff #28803)

This just got moved to line 84

121 ↗(On Diff #28803)

I tried using a Flatlist here; however, I wasn't able to get the elements to play nicely with the wrapping. However, I don't believe that we will be hurting performance by using map. Flatlists loads items that are currently visible on the screen and it deletes items as they go off screen, but I believe that in most cases for us, all all of these reactions for a message should always be visible on the screen at the same time so we won't really be able to take advantage of Flatlist's optimizations. Additionally, if you go further up the component tree, you will see that this entire component is part of a Flatlist renderItem in the message list component so the reactions for other messages shouldn't render until that Message component (which contains an InlineEngagement) is visible on the screen

external sources from react native docs and a blog from expo:

https://reactnative.dev/docs/flatlist#:~:text=To%20render%20multiple%20columns%2C%20use%20the%20numColumns%20prop.%20Using%20this%20approach%20instead%20of%20a%20flexWrap%20layout%20can%20prevent%20conflicts%20with%20the%20item%20height%20logic.

https://blog.expo.dev/react-native-flatlist-made-easy-20fca51e0327#:~:text=FlatList%20is%20a%20React%20Native%20component%20that%20only%20loads%20items%20that%20are%20currently%20visible%20on%20the%20screen%20and%20it%20deletes%20items%20as%20they%20go%20off%20screen

265 ↗(On Diff #28803)

The change here is that we change the name from inlineEngagementLeftStyle to inlineEngagementStyle

274 ↗(On Diff #28803)

The change here is that we change the name from inlineEngagementRightStyle to inlineEngagementStyle

ginsu requested review of this revision.Jul 18 2023, 3:17 PM
kamil added inline comments.
native/chat/inline-engagement.react.js
121 ↗(On Diff #28803)

sounds good to me

This revision is now accepted and ready to land.Jul 21 2023, 2:48 AM

fix center aligned inline engagement

native/chat/inline-engagement.react.js
170 ↗(On Diff #29031)

We have to explicitly use marginLeft here to override the marginLeft: avatarOffset in the inlineEngagement style