Page MenuHomePhabricator

[web] introduce member list sidebar provider
ClosedPublic

Authored by ginsu on Jan 31 2024, 1:15 PM.
Tags
None
Referenced Files
F3240292: D10899.id36473.diff
Wed, Nov 13, 12:19 PM
F3239738: D10899.id36473.diff
Wed, Nov 13, 10:22 AM
Unknown Object (File)
Sun, Nov 10, 1:02 AM
Unknown Object (File)
Fri, Nov 8, 10:03 PM
Unknown Object (File)
Fri, Nov 8, 7:16 PM
Unknown Object (File)
Fri, Nov 8, 7:16 PM
Unknown Object (File)
Fri, Nov 8, 7:16 PM
Unknown Object (File)
Fri, Nov 8, 7:15 PM
Subscribers

Details

Summary

The chat member list sidebar needs to be able to open + close from different points in the app. We will use showMemberListSidebar to track when the member list is open or close. Since we need to keep track of this state and use it many places, rather than prop drill this state to different places we should introduce a context provider that can be consumed in all the necessary parts

Linear task: https://linear.app/comm/issue/ENG-5975/introduce-new-member-list-section

Test Plan

Confirmed that the chat member list sidebar can open + close (will introduce the logic for that in subsequent diffs)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ginsu edited the test plan for this revision. (Show Details)
ginsu added reviewers: atul, inka.
ginsu requested review of this revision.Jan 31 2024, 1:34 PM
web/chat/member-list-sidebar/member-list-sidebar-provider.react.js
31 ↗(On Diff #36473)

Is it necessary to include setShowMemberListSidebar as a dependency?

web/app.react.js
219 ↗(On Diff #36473)

Is it necessary for this provider to be so high in the component hierarchy? Where will it be used? It looks like it should be inside Chat component (which is returned from getMainContentWithSwitcher)

web/app.react.js
219 ↗(On Diff #36473)

We will be using the state provided by MemberListSidebarProvider within the Chat component so MemberListSidebarProvider needs to sit above Chat in the component hierarchy. However, I updated the diff so that MemberListSidebarProvider is right above Chat making it lower in the component hierarchy

web/app.react.js
219 ↗(On Diff #36473)

EDIT: we should put MemberListSidebarProvider right above the renderMainContent and the modals. I just remembered that the user profiles will need to have context on this provider too. However, this should still be lower in the component hierarchy than from what I originally had

It seems odd that the modals would use this context. Can you give an example of how a modal will use it?

The implementation of MemberListSidebarProvider looks correct and makes sense for now.


That said, wonder if it would make sense to introduce some sort of higher level abstraction that's less MemberListSidebar specific. Similar to ModalContext, we could have something like PanelContext that could support multiple things being displayed in the sidebar, handle collapsing/displaying panels on the left, etc. Not sure if such scenarios exist in the designs, but seems like it'd be more flexible. Not necessary for now, but might be worth considering if we start introducing more sidebars.

This revision is now accepted and ready to land.Feb 5 2024, 10:39 AM

It seems odd that the modals would use this context. Can you give an example of how a modal will use it?

With user profiles we can navigate to our chat with that user. When we do this, we should use the context to close the members sidebar

Not sure if such scenarios exist in the designs, but seems like it'd be more flexible. Not necessary for now, but might be worth considering if we start introducing more sidebars.

Atm no such scenarios outside of the members list exist, but yea I totally agree, if we ever create new features that utilize a sidebar like this introducing a more generic PanelContext would make sense.

Going to land this diff for now since this feels outside the scope of what I have set, but will keep this in mind for when we next implement another panel sidebar and probably want to reuse a lot of this logic then.

This revision was landed with ongoing or failed builds.Feb 5 2024, 3:45 PM
This revision was automatically updated to reflect the committed changes.

With user profiles we can navigate to our chat with that user. When we do this, we should use the context to close the members sidebar

I see, thank you!