Page MenuHomePhabricator

[web] Add community drawer
ClosedPublic

Authored by inka on Jan 4 2023, 6:39 AM.
Tags
None
Referenced Files
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
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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

inka requested review of this revision.Jan 4 2023, 6:53 AM
web/sidebar/community-drawer-content.css
1 ↗(On Diff #20573)

I don't think we need to specify the type

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

I think there's a typo

43–52 ↗(On Diff #20573)

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.