[web] Extract from drawer item code that can be reused in community items
ClosedPublic
Actions

Authored by inka on Mar 7 2023, 3:37 AM.

Details

Reviewers

kamil
tomek
michal

Commits

rCOMMf612d842fe83: [web] Extract from drawer item code that can be reused in community items

Summary

isse: https://linear.app/comm/issue/ENG-3176/make-pressing-a-drawer-item-in-chat-tab-filter-available-chats
Because of changes to how the drawer is supposed to work I need to change how drawer items are implemented. Before, the only difference between items that represented community chats and other
chats was that community items shared the expanded state, as only one of them could be expanded at once, while other chats kept their own state each. This was implemented by having a
CommunityDrawerItem component that was wrapped by CommunityDrawerItemCommunity for community chats, and by CommunityDrawerItemChat for other chats. Those wrappers took care of the small
differences between the two types of drawer items.
But now, this task that I'm working on here makes the difference between the community items and other items so big, that it no longer makes sense for them to be wrappers of some shared
component. Insted they will be implemented as entierly separate components. The parts of the code that they can share are in this diff extraced from CommunityDrawerItem to
community-drawer-utils.react.js.

I wanted to just extract the common code in this diff, but to keep the code correct I had to introduce changes to CommunityDrawerItemCommunity as well, because of the following resons:

We cannot export two components form one file
CommunityDrawerItem and CommunityDrawerItemChat have to be in the same file
The code extracted from CommunityDrawerItem requires the CommunityDrawerItemChat component, while the old implementation of CommunityDrawerItemCommunity required

CommunityDrawerItem
That's why this diff is so large, and includes making CommunityDrawerItemCommunity an entierly separate component from CommunityDrawerItem. To not make this diff any bigger I left
CommunityDrawerItemChat and CommunityDrawerItem as separate components, but since now CommunityDrawerItemChat is the only component that uses CommunityDrawerItem, and has no problem being
merged with it, they will be merged in following diffs

Test Plan

Run web app,checked that drawer items work correctly: expand and navigate to chats in Chat tab, and change filters in Calendar tab. The highlight is supposed to not show up on community items,
since it will be entierly removed in the future, so I didn't implement it for the community items.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

inka created this revision.Mar 7 2023, 3:37 AM

Herald added subscribers: atul, ashoat. · View Herald TranscriptMar 7 2023, 3:38 AM

inka added a parent revision: D6972: [web] Make pressing Inbox button reset selected community.Mar 7 2023, 3:38 AM

inka added a child revision: D6974: [web] Add handlers for drawer community items.Mar 7 2023, 3:46 AM

Harbormaster completed remote builds in B17146: Diff 23486.Mar 7 2023, 3:53 AM

inka requested review of this revision.Mar 7 2023, 3:53 AM

ashoat added inline comments.Mar 7 2023, 6:23 AM

web/sidebar/community-drawer-utils.react.js
52–56 ↗	(On Diff #23486)	This is a lot of params, and three of them are booleans. You should pass in a single object instead

michal added inline comments.Mar 9 2023, 3:57 AM

web/sidebar/community-drawer-item-community.react.js
87 ↗	(On Diff #23486)	Can the `width: '100%' be moved to css?
90–93 ↗	(On Diff #23486)	`css.threadEntry` could be used directly.
web/sidebar/community-drawer-item.css
56 ↗	(On Diff #23486)	Why is this commented out?
web/sidebar/community-drawer-utils.react.js
26–28 ↗	(On Diff #23486)	Previously the check for `!itemChildren` was after the check for `hasSubchannelsButton`. Does this break anything?

tomek added inline comments.Mar 24 2023, 11:03 AM

web/sidebar/community-drawer-item-community.react.js
67–69 ↗	(On Diff #23486)	You can use a shorthand
web/sidebar/community-drawer-utils.react.js
68–80 ↗	(On Diff #23486)	You can consider making `onClick` nullable so that these two cases can become one.

ashoat added inline comments.Mar 24 2023, 1:49 PM

web/sidebar/community-drawer-utils.react.js
18–24 ↗	(On Diff #23486)	This would be good to be an object as well
24 ↗	(On Diff #23486)	Typical pattern for React components being passed around as variables is to use the standard `lowercaseVariableName`, and then to do something like this where it's used in JSX: const ComponentName = componentName; return <ComponentName />;

It seems like most of the comments request only simple changes, but there are a lot of them

This revision now requires changes to proceed.Mar 31 2023, 5:36 AM

inka added inline comments.Apr 7 2023, 6:59 AM

web/sidebar/community-drawer-utils.react.js
26–28 ↗	(On Diff #23486)	This condition actually wasn't used, because `!array` is not true when `array.length === 0`. But this is not a problem, because we just return an empty array that is handled in the same way as null.

Address review

Harbormaster completed remote builds in B18086: Diff 24787.Apr 7 2023, 7:16 AM

tomek accepted this revision.Apr 11 2023, 2:07 AM

tomek added 1 blocking reviewer(s): michal.

tomek added inline comments.

web/sidebar/community-drawer-utils.react.js
42	Should this be memoized as well?

Looks good to me

This revision is now accepted and ready to land.Apr 11 2023, 3:12 AM

Show avatars in top level drawer items

Harbormaster completed remote builds in B19574: Diff 26755.May 22 2023, 4:20 AM

inka added inline comments.May 22 2023, 5:54 AM

web/sidebar/community-drawer-utils.react.js
42	This is a function, not a component. The call to this function is memoized.

Closed by commit rCOMMf612d842fe83: [web] Extract from drawer item code that can be reused in community items (authored by • InkaSokolowska). · Explain WhyMay 23 2023, 1:01 AM

This revision was automatically updated to reflect the committed changes.

• InkaSokolowska added a commit: rCOMMf612d842fe83: [web] Extract from drawer item code that can be reused in community items.

ashoat added inline comments.May 23 2023, 5:38 AM

web/sidebar/community-drawer-item-community.react.js
60–61 ↗	(On Diff #26827)	Same pattern as mentioned elsewhere a couple times. I think you shouldn't be silencing this ESLint warning... it's pointing to the fact that you're specifying a parameter when you don't need to be