Page MenuHomePhabricator

[web] Add community drawer

Authored by inka on Jan 4 2023, 6:39 AM.
Referenced Files
Unknown Object (File)
Mon, Feb 27, 5:57 PM
Unknown Object (File)
Sat, Feb 25, 8:54 AM
Unknown Object (File)
Feb 21 2023, 3:16 AM
Unknown Object (File)
Feb 20 2023, 3:10 PM
Unknown Object (File)
Feb 17 2023, 12:02 AM
Unknown Object (File)
Feb 15 2023, 10:19 PM
Unknown Object (File)
Feb 13 2023, 4:34 PM
Unknown Object (File)
Feb 10 2023, 3:02 AM



Linear issue:
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

rCOMM Comm
Lint Not Applicable
Tests Not Applicable

Event Timeline

inka requested review of this revision.Jan 4 2023, 6:53 AM
1 ↗(On Diff #20573)

I don't think we need to specify the type

39 ↗(On Diff #20573)

I think there's a typo

43–52 ↗(On Diff #20573)

This can be simplified

39–41 ↗(On Diff #20676)

I'm a bit confused by index name, on callsites of toggleExpanded we always pass 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?

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

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.
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
4 ↗(On Diff #21675)

We definitely should as it provides types for AppState

This revision was automatically updated to reflect the committed changes.