Page MenuHomePhabricator

[web] Make drawer items not expandable in the calendar tab
ClosedPublic

Authored by inka on Jan 24 2023, 10:33 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Dec 23, 11:51 AM
Unknown Object (File)
Fri, Dec 20, 4:05 PM
Unknown Object (File)
Fri, Dec 20, 3:27 PM
Unknown Object (File)
Fri, Dec 20, 3:27 PM
Unknown Object (File)
Fri, Dec 20, 3:20 PM
Unknown Object (File)
Fri, Dec 20, 3:16 PM
Unknown Object (File)
Fri, Dec 20, 3:03 PM
Unknown Object (File)
Fri, Dec 20, 3:01 PM
Subscribers

Details

Summary

issue: https://linear.app/comm/issue/ENG-2789/make-drawer-items-in-calendar-tab-not-expandable
The new designs specify, that drawer items are not supposed to be expandable in the Calendar tab.

Test Plan

Run the web app, check that drawer items are not expandable in the Calendar tab, but stay expandable in outher tabs.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

web/sidebar/community-drawer-content.react.js
22 ↗(On Diff #21255)

Max depth defines how many levels of chats will be displayed in the drawer. In the calendar tab, we want to display only the communities

51 ↗(On Diff #21255)

For now assuming, that we will never want to display only communities, and have them expand to subchannels buttons. That would be rather silly. The code we already have doesn't even allow this.

inka requested review of this revision.Jan 24 2023, 10:48 AM

Remove falsy dark background in the Calendar tab

tomek requested changes to this revision.Jan 26 2023, 4:46 AM
tomek added inline comments.
web/sidebar/community-drawer-content.react.js
51 ↗(On Diff #21256)

We could also use expandable as a name. Usually, it doesn't make a big difference, but it's nicer to see <CommunityDrawerItemCommunity expandable /> than <CommunityDrawerItemCommunity allowExpanding />

web/sidebar/community-drawer-item-community.react.js
13 ↗(On Diff #21256)

This feels like a hack. Can we modify the code so that when allowExpanding is false, all the communities have expanded = false? Probably

const [openCommunity, setOpenCommunity] = React.useState(
  communitiesSuffixed.length === 1 ? communitiesSuffixed[0].id : null,
);

should become something like this

const [openCommunity, setOpenCommunity] = React.useState(
  communitiesSuffixed.length === 1 && maxDepth > 0? communitiesSuffixed[0].id : null,
);

We can also consider introducing a new variable

const expandable = maxDepth > 0;
This revision now requires changes to proceed.Jan 26 2023, 4:46 AM

This solution doesn't preserve the state (expanded/collapsed) of items on deeper levels. ie if I expand chats from levels 0,1,2, then navigate to Calendar, then navigate back to Chat, the only item that is expanded is the community. So I will have to change my approach.
The best solution I found for now is having two lists of CommunityDrawerItems in the CommunityDrawerContent (or two CommunityDrawerContents in the CommunityDrawer), and rendering both of them every time, just setting one of them to have display: none or visibility: hidden. This is not a great solution, it doesn't scale well if we wanted the drawer to display in a yet another way for another app. But the drawer items can still trigger different behaviours in different apps with this approach, just the way the drawer is displayed is limited. And I think we will not want the way the drawer looks to change for each app. I think the "always collapsed" and "expandable" options probably cover all that we need. So I will go with this solution for now, and if in the future we decide it's not enough - that we want the drawer to be able to take many different forms - then it will possibly have to be changed.

Change strategy to preserve drawer state from one app, when switching to another tab

The state is preserved now

Remove accidental changes

tomek added inline comments.
web/sidebar/community-drawer.react.js
69 ↗(On Diff #22118)

Can we make this an optional prop and don't provide it here instead of using an empty function?

78–85 ↗(On Diff #22118)

You can create a class e.g. .hidden and use it conditionally.

This revision is now accepted and ready to land.Feb 7 2023, 8:28 AM