Page MenuHomePhabricator

[native] Extract from SidebarListModal code that can be reused for the subchannels modal
ClosedPublic

Authored by inka on Dec 21 2022, 7:10 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 6, 3:35 PM
Unknown Object (File)
Mon, Nov 4, 8:30 AM
Unknown Object (File)
Mon, Nov 4, 8:30 AM
Unknown Object (File)
Mon, Nov 4, 3:16 AM
Unknown Object (File)
Mon, Nov 4, 1:14 AM
Unknown Object (File)
Mon, Oct 28, 7:04 AM
Unknown Object (File)
Mon, Oct 28, 7:04 AM
Unknown Object (File)
Mon, Oct 28, 7:04 AM
Subscribers

Details

Summary

Related Linear task: https://linear.app/comm/issue/ENG-2373/subchannels-view
The subchannels modal is supposed to be similar to SidebarListModal, so I extracted from it the code that can be reused.

Test Plan

Run ios simulator, see that sidebar modal works

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

inka requested review of this revision.Dec 21 2022, 7:22 AM
native/chat/sidebar-list-modal.react.js
39–43 ↗(On Diff #19950)

Since the onPressItem function is the same for the sidebars modal and the subchannels modal, but renderItem is not (because of the checks that need to be performed in the following lines), I decided to inject onPressItem to the renderItem function. So this is a function that takes onPressItem as an argument, and returns a renderItem function based on it.

native/chat/thread-list-modal.react.js
67–79 ↗(On Diff #19950)

onPressItem uses state internal to the component, so it cannot be moved out of it.

kamil requested changes to this revision.Dec 22 2022, 6:31 AM
kamil added inline comments.
native/chat/thread-list-modal.react.js
37 ↗(On Diff #19950)

why the type is not SetState<ThreadSearchState>?

66 ↗(On Diff #19950)

Are you sure we should use it here? Not pass callback from parent component how to handle navigating?

81 ↗(On Diff #19950)

It will work but since renderItem is defined as a function I would stick to React.useCallback

93 ↗(On Diff #19950)

Maybe I am missing something but are we really want to show this placeholder even for searching for threads?

This revision now requires changes to proceed.Dec 22 2022, 6:31 AM
native/chat/thread-list-modal.react.js
66 ↗(On Diff #19950)

Yeah, I really don't think anyone will want to use this modal to navigate somewhere else than to a thread.

81 ↗(On Diff #19950)

We have an eslint rule that says useCallback has to be passed an inline. It seems that the community uses useMemo in cases like this.

kamil added a reviewer: tomek.
tomek added inline comments.
native/chat/sidebar-list-modal.react.js
34–37 ↗(On Diff #20148)

It is just a number so it doesn't need to be a memo

native/chat/thread-list-modal.react.js
31–35 ↗(On Diff #20148)

Can we use read only here?

This revision is now accepted and ready to land.Dec 29 2022, 12:47 AM

Address code review, rebase