Chat component refactor

[lourou]

Refactor Chat component and optimize message handling for chat preview (long press on chat > auxiliary preview)

• Remove the readOnly props in the chat component, and create a dedicated, more lightweight <ChatPreview /> component instead
• Extract usePeerSocials, renderItem and getItemType
• Add optional lastMessages parameter in getListArray to limit the number of returned messages in <ChatPreview />
• Remove readOnly prop from Chat component, simplifying logic for padding and focus states
• Replace ConverseChat with the newly refactored Chat component
• Remove default export
• Remove unused code in screens/ConversationReadOnly.tsx (onLeaveScreen)

Linked to: #819 (comment)

[nmalzieu]

Nice! Just a small question about duplicated code

[nmalzieu]

I like that we cleaned the component and extracted some hooks & methods from it but still there is a lot of copy/paste here. why do we feel a new component is better than a readOnly parameter? do we need to maintain both?

[thierryskoda]

Agree that doing optional props or using composable pattern can have a fine line.

I also don't often like composable when we know that most of the things we extract we know we won't reuse them many time in the app.

But I think for the animation performance issue it's still worth it to do it here.

It's also easier to understand and isolate component in that way instead of having so many different ways a component can render and react.

Optional props are really not recommended for that. Especially in big component.

I also think that 1-2 lines hooks like those for example are perfectly fine and actually perfect way to make our component composable.

const peerSocials = usePeerSocials();
const isSplitScreen = useIsSplitScreen();

We could even add more to make sure that if we change something in our chat we only change it once by adding:

function useAnimatedListView(conversation) {
  const conversationNotPendingRef = useRef(
    conversation && !conversation.pending
  );

  const AnimatedListView =
    conversationNotPendingRef.current && Platform.OS !== "web"
      ? ReanimatedFlashList
      : ReanimatedFlatList;

  return AnimatedListView;
}

And

function useIsShowingPlaceholder(listArray, isBlockedPeer, conversation) {
  return listArray.length === 0 || isBlockedPeer || !conversation;
}

Just my opinion. Happy to chat more about that.

[alexrisch]

Further I think that needs to be memoized in some way, I'm pretty sure

const AnimatedListView =
    conversationNotPendingRef.current && Platform.OS !== "web"
      ? ReanimatedFlashList
      : ReanimatedFlatList;

Is causing a large bit of rerendering since it's a new component each time

[lourou]

Thanks @nmalzieu @thierryskoda and @alexrisch – I've just pushed an update with more custom hooks, also memoized the AnimatedListView

[thierryskoda]

I like that when there's more than 1 argument we use object arguments instead. But my own opinion. Not sure if we have some specifics docs around that.

[thierryskoda]

For a hook like that I would do this. (again just my own opinion lol)

const useIsShowingPlaceholder = (args: {
  listArray: MessageToDisplay[];
  isBlockedPeer: boolean;
  hasConversation: boolean;
}): boolean => {
  return args.listArray.length === 0 || args.isBlockedPeer || ! args.hasConversation;
};

[lourou]

Hey @thierryskoda

Thanks for your feedback. As you suggested, I've refactored useIsShowingPlaceholder and useRenderItem to use the object parameter pattern. I did direct destructuring in the function parameters rather than using 'args'.

Cheers!