Page MenuHomePhabricator

[native] Add subchannnels modal
ClosedPublic

Authored by inka on Dec 21 2022, 7:21 AM.
Tags
None
Referenced Files
F3353156: D5972.diff
Sat, Nov 23, 8:42 AM
Unknown Object (File)
Tue, Nov 12, 1:21 AM
Unknown Object (File)
Sun, Nov 10, 5:37 PM
Unknown Object (File)
Wed, Nov 6, 6:30 AM
Unknown Object (File)
Mon, Nov 4, 1:14 AM
Unknown Object (File)
Mon, Nov 4, 1:14 AM
Unknown Object (File)
Mon, Nov 4, 1:14 AM
Unknown Object (File)
Mon, Nov 4, 1:14 AM
Subscribers

Details

Summary

Linear issue: https://linear.app/comm/issue/ENG-2373/subchannels-view
Adding a subchannels modal, similar to the SidebarListModal that displays sidebars of a chat.
This modal is supposed to display subchannels of a given chat. It is to be used in the community drawer.

As the designs are still being discussed, this diff doesn't introduce the final styling yet. Here is what it looks like for now:

image.png (1×612 px, 114 KB)

Test Plan

In chat-thread-list.react in onPressSeeMoreSidebars exchange the navigation call for:

this.props.navigation.navigate<'SubchannelsListModal'>({
      name: SubchannelsListModalRouteName,
      params: { threadInfo },
    });

Run ios simulator, press “See more…” under a chat that has threads and see that the SubchannelsListModal appears

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

inka edited the test plan for this revision. (Show Details)
inka requested review of this revision.Dec 21 2022, 7:33 AM
kamil added a reviewer: tomek.
kamil added inline comments.
native/chat/subchannel-item.react.js
66 ↗(On Diff #19954)

I am not sure about the need for this const and exporting this but if you are sure it is needed or will be used in future diffs maybe let's move it to chat-constants.js?

115 ↗(On Diff #19954)

Maybe I am wrong but I think that for components we use export default and if you want to import additional thing just put export inline

native/chat/subchannel-item.react.js
115 ↗(On Diff #19954)

Exporting anything other than a React component from a file that exports a React component will break hot reloading. See https://twitter.com/benschac/status/1521867664267821063, https://phab.comm.dev/D3972, https://reactnative.dev/docs/fast-refresh#how-it-works

native/chat/subchannel-item.react.js
115 ↗(On Diff #19954)

Sorry, that's an artefact, I'll remove that.
But on that note, should we change this then?

native/chat/subchannel-item.react.js
115 ↗(On Diff #19954)

Yeah that would be great in a separate diff!

tomek requested changes to this revision.Dec 23 2022, 4:41 AM

Requesting changes because exports need to be fixed, but overall looks good!

I was concerned about createRenderItem pattern - it sounded strange, but after some thought it makes sense.

native/chat/subchannels-list-modal.react.js
33–41 ↗(On Diff #19954)

This doesn't change and can be defined outside the component.

This revision now requires changes to proceed.Dec 23 2022, 4:41 AM

Address code review: fix exports, move createRenderItem function outside of the component. Additional changes
due to changes in previous diff.

native/chat/subchannel-item.react.js
115 ↗(On Diff #19954)
native/chat/subchannels-list-modal.react.js
41 ↗(On Diff #20149)

Due to changes in previous diff

This revision is now accepted and ready to land.Dec 27 2022, 5:06 AM

Rebase + changes due to changes in previous diff