Page MenuHomePhabricator

[web] Introduce `Leave` thread menu action
ClosedPublic

Authored by jacek on Feb 14 2022, 3:47 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Dec 4, 1:32 AM
Unknown Object (File)
Nov 16 2024, 5:34 AM
Unknown Object (File)
Nov 16 2024, 3:04 AM
Unknown Object (File)
Nov 9 2024, 10:46 PM
Unknown Object (File)
Nov 9 2024, 10:46 PM
Unknown Object (File)
Nov 2 2024, 6:16 AM
Unknown Object (File)
Oct 7 2024, 2:59 AM
Unknown Object (File)
Oct 7 2024, 2:59 AM

Details

Summary

Added Leave Thread option.
It's functionality after click will be added in separate diff.

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

Test Plan

Tested if the item is displayed in threads, that can be left. It shouldn't appear in Personal threads.

Diff Detail

Repository
rCOMM Comm
Branch
jacek/thread-menu
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

tomek requested changes to this revision.Feb 14 2022, 5:08 AM
tomek added inline comments.
web/chat/thread-menu.css
58 ↗(On Diff #9602)

My guess is that we should specify a margin instead of max-width, so that when there's a really long item, or all the items are short, the layout still looks ok. We can use both max-with and margin just to make sure that the separator is never too short.

web/chat/thread-menu.react.js
119 ↗(On Diff #9602)

We shouldn't do it like that. If there are more items from the second section it would be hard to determine if we need a separator. We should instead check in menuItems if there are items from the second section and either render the first section, or first section + separator + second section. This breaks my idea of avoiding repetition in menuItems.

This revision now requires changes to proceed.Feb 14 2022, 5:08 AM
web/chat/thread-menu.css
58 ↗(On Diff #9602)

My guess is that we should specify a margin instead of max-width, so that when there's a really long item, or all the items are short, the layout still looks ok.

It is exactly what I did.

We can use both max-with and margin just to make sure that the separator is never too short.

How the use of max-width would make sure it is long enough? Did you mean min-width?

Personally, I think it looks ok as it's now.

web/chat/thread-menu.react.js
119 ↗(On Diff #9602)

I haven't seen any design of the thread menu in Figma, that would require additional items below the separator, except the "Leave Thread" one.
If we'd like to reuse the component (what I'm going to do in separate diff), maybe it would be a good idea not to divide items into only two sections, as we may need more separators or sections in other place in app?
I don't feel what exactly is wrong with current solution? Could you be more detailed?

web/chat/thread-menu.css
58 ↗(On Diff #9602)

You're totally right, sorry for the confusion. I guess adding min-width might be a good idea though.

web/chat/thread-menu.react.js
119 ↗(On Diff #9602)

The issue with current solution is that leaveThreadItem consists of two parts, one of them is the separator. The problem is that we can't say that the separator is a part of the item and at the same time the code is telling exactly that. So the relationship is more like "something is next to something" and not "something is a part of something".
The separator is not a part of any item and instead it is a part of the menu, so it's more intuitive to define it as a part of it. Putting the things where they truly belong improves the readability as you can use intuition from the real world.

Regarding the designs, it is true that we don't have that, but at the same time it would be much easier to add such a thing in the future if the separator was defined inside the menu. The reason is that the leaveThreadItem is rendered conditionally and it is possible that someone is going to miss the fact that it renders both an item and a separator. This could lead to bugs with missing / duplicate separator.

As a side note, when you need to use Fragment, but don't need to give it any props, there's a shorthand for that <>.

tomek requested changes to this revision.Feb 16 2022, 6:31 AM
This revision now requires changes to proceed.Feb 16 2022, 6:31 AM

move separator, after Tomek's suggestion

This revision is now accepted and ready to land.Feb 21 2022, 4:49 AM
This revision now requires review to proceed.Feb 21 2022, 4:49 AM

Updated separator CSS to match Figma

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