Page MenuHomePhabricator

[web] Make drawer items react based on the current app

Authored by inka on Jan 18 2023, 12:39 AM.
Referenced Files
Unknown Object (File)
Fri, Mar 3, 8:50 PM
Unknown Object (File)
Wed, Mar 1, 8:40 AM
Unknown Object (File)
Wed, Mar 1, 3:38 AM
Unknown Object (File)
Tue, Feb 28, 1:04 PM
Unknown Object (File)
Tue, Feb 28, 11:04 AM
Unknown Object (File)
Tue, Feb 28, 9:24 AM
Unknown Object (File)
Tue, Feb 28, 9:12 AM
Unknown Object (File)
Tue, Feb 28, 1:28 AM



Linear issue:
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.
For each App we either implement a Handler component, or it'll use the default Handler. We obtain these Handlers via getCommunityDrawerItemHandler function. The Handler component doesn't
render anything, but sets a handler object in the drawer item via setHandler. 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). For now, it implements the "onClick" function.

Test Plan

Run web app, check that pressing items in the drawer results in the behaviour I described above, based on the tab the user is in.

Diff Detail

rCOMM Comm
Lint Not Applicable
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Jan 18 2023, 12:42 AM
Harbormaster failed remote builds in B15425: Diff 21034!
inka requested review of this revision.Jan 19 2023, 1:57 AM
tomek requested changes to this revision.Jan 19 2023, 3:51 AM
tomek added inline comments.
73–76 ↗(On Diff #21074)

We don't need to memoize the Handler because it can only return a couple of const values which aren't created inside getCommunityDrawerItemHandler. But this is an implementation detail of getCommunityDrawerItemHandler and relying on that has a tiny chance of causing issues in the future.

A better idea might be to call this function as a part of the selector. This has an additional benefit of a possible performance optimization because there are multiple tabs for which the same handler value is provided.

79–80 ↗(On Diff #21074)

Why do we need to silence this message? Would removing a parameter solve the issue?

85 ↗(On Diff #21074)

Are we sure that these conditions remain correct for all the other apps? What happens when we are creating a chat and then decide to open a calendar? Will the drawer be useful?

This revision now requires changes to proceed.Jan 19 2023, 3:51 AM
79–80 ↗(On Diff #21074)

If I do that Flow doesn't allow me to use this function in line 86, since the function does not expect an argument and i'm passing it an event

85 ↗(On Diff #21074)

I started to think about this, and actually here is what happens for ChatThreadList, where I took this code from:


I suppose this is a bug?

Anyway this condition means that we cannot navigate to a thread if it is both active and in create mode.
We can navigate to other chats, so the drawer works fine in this scenario. But actually I don't display chats that are being created in the drawer, so this is unnecessary.
But by the way, this also means that when we are in a chat (that has already been created), and press on it in the ChatThreadList, we still dispatch a UPDATE_NAV_INFO. Do we want that to happen?

tomek added inline comments.
91 ↗(On Diff #21146)

When the onClick callback is so simple, there's no need to create it at all.

85 ↗(On Diff #21074)

Yes, it is a bug - could you create a task for it?

Regarding the additional dispatch, if it doesn't affect the history, it doesn't matter that much. But it might be a good idea to make the current thread unclickable.

This revision is now accepted and ready to land.Jan 23 2023, 8:47 AM
85 ↗(On Diff #21074)