Paths

Table of Contentst

[web] Introduce `Subchannels` thread menu actions
ClosedPublic
Actions

Authored by • jacek on Feb 14 2022, 3:44 AM.

Details

Reviewers

tomek
• benschac
atul
ashoat

Commits

rCOMM870e9da5aced: [web] Introduce `Subchannels` thread menu actions

Summary

Added Subchannels list and create options. It appears depending on user permissions - in the same way as in native.
The onClick functionality will be added in a separate diff.

Screenshot_Google Chrome_2022-02-14_122438.png (1×880 px, 89 KB)

Test Plan

Tested if items appears if user has proper permissions in thread.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• jacek created this revision.Feb 14 2022, 3:44 AM

Herald added subscribers: • karol, atul, • adrian and 2 others. · View Herald TranscriptFeb 14 2022, 3:44 AM

Harbormaster completed remote builds in B6779: Diff 9601.Feb 14 2022, 3:50 AM

• jacek requested review of this revision.Feb 14 2022, 3:50 AM

• jacek edited the summary of this revision. (Show Details)Feb 14 2022, 3:51 AM

• jacek added a reviewer: tomek.

• jacek edited the summary of this revision. (Show Details)

• jacek added a parent revision: D3188: [web] Introduce `ThreadMenuItem`.

• jacek added reviewers: • benschac, atul.Feb 14 2022, 3:55 AM

tomek requested changes to this revision.Feb 14 2022, 5:03 AM

tomek added inline comments.

web/chat/thread-menu.react.js
76–93 ↗	(On Diff #9601)	We could compute this value outside of this memo and call it e.g. `hasSubchannels` (it will be another memo). The same can be applied for `sidebarItem` / `hasSidebars`. The benefit is that we decide if we need to render based on a fact that a threads has or hasn't subchannels, and we don't care which are these. So it doesn't make sense to rerender when a new subchannel appeared or when one of subchannels has changed.

This revision now requires changes to proceed.Feb 14 2022, 5:03 AM

• jacek added inline comments.Feb 14 2022, 6:44 AM

web/chat/thread-menu.react.js
76–93 ↗	(On Diff #9601)	Thanks for the catch. I fixed it also for sidebars.

Add hasSubchannels useMemo following Tomek's advice.

Harbormaster completed remote builds in B6794: Diff 9619.Feb 14 2022, 6:50 AM

tomek accepted this revision.Feb 14 2022, 7:34 AM

This revision is now accepted and ready to land.Feb 14 2022, 7:34 AM

Herald added a reviewer: ashoat. · View Herald TranscriptFeb 14 2022, 7:34 AM

This revision now requires review to proceed.Feb 14 2022, 7:34 AM

ashoat requested changes to this revision.Feb 14 2022, 9:15 PM

ashoat added inline comments.

web/chat/thread-menu.react.js
82–86 ↗	(On Diff #9619)	I think this is more clear
84 ↗	(On Diff #9619)	We should avoid making direct comparisons like this because it will make our lives difficult in the future if we want to change what the definition of a subchannel is. Instead, let's use `threadIsTopLevel` here.

This revision now requires changes to proceed.Feb 14 2022, 9:15 PM

ashoat added inline comments.Feb 14 2022, 9:17 PM

web/chat/thread-menu.react.js
84 ↗	(On Diff #9619)	Actually, `threadIsTopLevel` is wrong... Instead, we should factor out a function called `threadIsChannel` that is the same as `threadIsTopLevel` but doesn't check `threadInChatList`. And then have both `threadIsTopLevel` and this spot call our new function. The new function should be introduced in a new diff directly preceding this one.

ashoat mentioned this in D3190: [web] Introduce `Members` and `Sidebars` thread menu actions.Feb 14 2022, 9:19 PM

• jacek added inline comments.Feb 16 2022, 3:11 AM

web/chat/thread-menu.react.js
84 ↗	(On Diff #9619)	I used here exactly the same condition, that is currently in mobile app. I suggest leaving the current - correctly working - solution in this diff, and then a follow-up diff refactoring the condition (and introducing `threadIsChannel`) on both web and mobile, so we don't have differences between platforms.

Simplified logic using some instead of filter

Harbormaster completed remote builds in B6877: Diff 9725.Feb 16 2022, 6:21 AM

That approach is totally fine, but you should not re-request review without FIRST doing one of the below:

Creating a Linear task to track AND link it in the diff comment here, OR
Creating a diff that resolves the concern AND link it in the diff comment here

Will let it slide this time, but please keep this in mind going forward, and do NOT land this without first addressing the above (link the follow-up!)

This revision is now accepted and ready to land.Feb 21 2022, 10:45 PM

Note: you should've specified your dependency graph here by including "Depends on: D3190" in the diff description. Please keep this in mind – going forward, I'm going to start instantly requesting changes whenever I see diff dependencies not specified from anybody on the team. The goal is to move as much of the diff reviewers' work to the diff author as possible

In D3191#86888, @ashoat wrote:

Note: you should've specified your dependency graph here by including "Depends on: D3190" in the diff description. Please keep this in mind – going forward, I'm going to start instantly requesting changes whenever I see diff dependencies not specified from anybody on the team. The goal is to move as much of the diff reviewers' work to the diff author as possible

I specify dependencies using "Edit related revisions" menu in right panel. Dependency graph is visible in "Stack" tab in "Revision Contents" panel. I think it works in the same way as adding "Depends on..." in description.

• jacek mentioned this in D3261: [web, native] Introduce and use `ThreadIsChannel` function.Feb 22 2022, 4:50 AM

• jacek added a child revision: D3261: [web, native] Introduce and use `ThreadIsChannel` function.Feb 22 2022, 4:56 AM

In D3191#86882, @ashoat wrote:

That approach is totally fine, but you should not re-request review without FIRST doing one of the below:

Creating a Linear task to track AND link it in the diff comment here, OR

Creating a diff that resolves the concern AND link it in the diff comment here

Will let it slide this time, but please keep this in mind going forward, and do NOT land this without first addressing the above (link the follow-up!)

Created a diff D3261, that follows your suggestion

I specify dependencies using "Edit related revisions" menu in right panel. Dependency graph is visible in "Stack" tab in "Revision Contents" panel. I think it works in the same way as adding "Depends on..." in description.

Sorry, this was big oversight on my part. No idea what led to my confusion last night. Probably was just a lack of sleep... my bad!