Page MenuHomePhabricator

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

Authored by inka on Mar 7 2023, 3:37 AM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 21 2024, 4:05 AM
Unknown Object (File)
Feb 21 2024, 1:43 AM
Unknown Object (File)
Feb 21 2024, 1:43 AM
Unknown Object (File)
Feb 21 2024, 1:43 AM
Unknown Object (File)
Feb 21 2024, 1:43 AM
Unknown Object (File)
Feb 21 2024, 1:43 AM
Unknown Object (File)
Feb 21 2024, 12:33 AM
Unknown Object (File)
Feb 20 2024, 11:14 PM
Subscribers

Details

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:

  1. We cannot export two components form one file
  2. CommunityDrawerItem and CommunityDrawerItemChat have to be in the same file
  3. 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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

inka requested review of this revision.Mar 7 2023, 3:53 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

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?

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.

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 />;
tomek requested changes to this revision.Mar 31 2023, 5:36 AM

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
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.

tomek added 1 blocking reviewer(s): michal.
tomek added inline comments.
web/sidebar/community-drawer-utils.react.js
42 ↗(On Diff #24787)

Should this be memoized as well?

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

Show avatars in top level drawer items

web/sidebar/community-drawer-utils.react.js
42 ↗(On Diff #24787)

This is a function, not a component. The call to this function is memoized.

web/sidebar/community-drawer-item-community.react.js
60–61

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