Page MenuHomePhabricator

[web] Add community drawer
ClosedPublic

Authored by inka on Jan 4 2023, 6:39 AM.
Tags
None
Referenced Files
F3393432: D6162.id22469.diff
Sat, Nov 30, 1:52 PM
F3393367: D6162.id22270.diff
Sat, Nov 30, 1:31 PM
F3393204: D6162.id21672.diff
Sat, Nov 30, 12:25 PM
F3393169: D6162.id20676.diff
Sat, Nov 30, 12:07 PM
F3393089: D6162.diff
Sat, Nov 30, 11:28 AM
F3392813: D6162.id22270.diff
Sat, Nov 30, 10:08 AM
Unknown Object (File)
Thu, Nov 28, 11:29 AM
Unknown Object (File)
Thu, Nov 28, 11:29 AM
Subscribers

Details

Summary

Linear issue: https://linear.app/comm/issue/ENG-2631/drawer-sidebar-inside-of-community-picker
Adding the community drawer.
Adding the community drawer on native: D5723 and D5732

Test Plan

added community drawer to AppsDirectory, checked that it behaves as expected.

Diff Detail

Repository
rCOMM Comm
Branch
inka/community_drawer_web
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

inka requested review of this revision.Jan 4 2023, 6:53 AM
web/sidebar/community-drawer-content.css
1

I don't think we need to specify the type

web/sidebar/community-drawer-content.react.js
39

I think there's a typo

43–52

This can be simplified

web/sidebar/community-drawer-content.react.js
39–41 ↗(On Diff #20676)

I'm a bit confused by index name, on callsites of toggleExpanded we always pass threadInfo.id to this function. Maybe we rename it to communityRootThreadId or something like that? I have limited understanding of that recursive data structure, but from what I've read we represent communities by threadIDs of their roots, right?

web/sidebar/community-drawer.react.js
8–14 ↗(On Diff #20676)

Is there a reason we've introduce new component just for wrapping it in a Provider?
Maybe we can rename CommunityDrawerContent to CommunityDrawer and wrap <div> it returns with the Provider

web/sidebar/community-drawer.react.js
8–14 ↗(On Diff #20676)

It was mostly done to keep a similar structure to what we have on native, where the counterpart to CommunityDrawer : CommunityDrawerNavigator has a lot of logic. But I suppose Community Picker can be considered the counterpart to CommunityDrawerNavigator. I'll change that, thank you

Address code review, remove CommunityDrawer

inka planned changes to this revision.Jan 31 2023, 5:00 AM

Fix having accidentally applied changes from future diffs

tomek added inline comments.
web/sidebar/community-drawer.react.js
4 ↗(On Diff #21675)

Should we use utils/redux-utils instead?

44–51 ↗(On Diff #21675)

Should we memoize this?

This revision is now accepted and ready to land.Jan 31 2023, 6:58 AM
web/sidebar/community-drawer.react.js
4 ↗(On Diff #21675)

We definitely should as it provides types for AppState

This revision was automatically updated to reflect the committed changes.