Page MenuHomePhabricator

[web] Add community drawer item handlers
ClosedPublic

Authored by inka on Jan 17 2023, 7:26 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 25, 3:36 PM
Unknown Object (File)
Oct 28 2024, 8:11 AM
Unknown Object (File)
Oct 18 2024, 10:54 AM
Unknown Object (File)
Oct 2 2024, 3:43 PM
Unknown Object (File)
Sep 28 2024, 1:03 PM
Unknown Object (File)
Sep 28 2024, 1:03 PM
Unknown Object (File)
Sep 28 2024, 1:03 PM
Unknown Object (File)
Sep 28 2024, 1:03 PM
Subscribers

Details

Summary

Linear issue: https://linear.app/comm/issue/ENG-2650/make-the-calendar-change-on-community-drawer-action
Pressing an item in the community drawer on web should result in different behaviour depending on the app the user has currently open. In chat it is supposed to open the corresponding chat. In calendar it is supposed to update the filters so that only the events of the corresponding chat are displayed. In Apps it is supposed to navigate back to the Chat tab and open the corresponding chat.
We took the following approach: we have a "map" from tab names to Handler components. The component takes a 'setHandler' function from its parent. The component performs some operations - like calling hooks, and creates a handler object. It then puts the handler object via setHandler in the parent component's state. This handler object is of CommunityDrawerItemHandlerSpec type, so it implements all functions that are needed by the drawer in each tab (that are specific to the tab).

Lastly - the access to this "map" of Handlers is gated by the getCommunityDrawerItemSpec function, because we assume there might be more apps in the future, and some of them (like Apps tab for now) might not want to have any custom behaviour defined for pressing a drawer item, but use the default - navigating to Chat tab and opening the corresponding chat.
getCommunityDrawerItemSpec function takes care of that "fallback", so that the calling code doesn't have to consider this.

Test Plan

Check that getCommunityDrawerItemSpec returns the correct component for each app by hand running the handler is sets for the parent.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Jan 17 2023, 7:40 AM
Harbormaster failed remote builds in B15417: Diff 21019!
inka requested review of this revision.Jan 17 2023, 7:52 AM
inka retitled this revision from [web] Add community drawer item specs to [web] Add community drawer item handlers.Jan 17 2023, 7:55 AM
web/sidebar/community-drawer-item-handlers.react.js
57 ↗(On Diff #21020)

This syntax can be $ReadOnly too

web/sidebar/community-drawer-item-handlers.react.js
36–46 ↗(On Diff #21020)

Based on what happens when we press "only" in calendar filters next to a chat item: source code

tomek requested changes to this revision.Jan 18 2023, 5:13 AM

Requesting changes because there are a couple of small issues, but overall looks great!

web/sidebar/community-drawer-item-handlers.react.js
27 ↗(On Diff #21020)

We don't need to specify the transitive dependencies

51 ↗(On Diff #21020)

We don't need to specify the transitive dependencies

56–70 ↗(On Diff #21020)

Seems a little risky to include default as a key. It is unlikely, but if we introduce an app with default as a name, it could cause some issues.

57 ↗(On Diff #21020)

Can we limit the available keys by using NavigationTab from nav-types?

web/sidebar/community-drawer-item-spec.react.js
3 ↗(On Diff #21020)

We can simplify the name

This revision now requires changes to proceed.Jan 18 2023, 5:13 AM

Address code review + rename community-drawer-item-spec.react.js to community-drawer-item-handler.react.js

This revision is now accepted and ready to land.Jan 19 2023, 2:10 AM