Introduce top bar thread menu. Currently, without any actions.
Menu disappears after clicking outside.
The eslint-ignore is only for current diff, as the following will add actions that use ThreadInfo.
Details
Details
- Reviewers
tomek • benschac atul ashoat - Commits
- rCOMM58735acd48ff: [web] Introduce `ThreadMenu`
Tested the menu with actions (in following diffs) in many different thread types, to make sure proper items are displayed for each thread.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
Interesting approach. I think this is better than what we've done for thread list dropdown (the one with set read / unread). We probably should use the same approach there... maybe we can extract a component / hook that can be reused there?
| web/chat/thread-menu.react.js | ||
|---|---|---|
| 18–19 ↗ | (On Diff #9596) | You could as well just delete these lines |
| 27 ↗ | (On Diff #9596) | Is it really clickOutside? My guess is that this callback will be called on any click, and not just outside. |
| 28 ↗ | (On Diff #9596) | We should prefer attaching listeners to document - for compatibility reasons |
| 50 ↗ | (On Diff #9596) | This should be a callback |
| 51 ↗ | (On Diff #9596) | Are you sure this code is called and not the code from clickOutsideCallback? |
| web/chat/thread-menu.react.js | ||
|---|---|---|
| 51 ↗ | (On Diff #9596) | Both callbacks are called. I noticed, that this one is executed always before the clickOutsideCallback, so it works correctly. |
Comment Actions
I can exclude the component into the common one for both places in a follow-up diff.
Comment Actions
Renamed clickOutside..., replaced window with document and created switchMenuCallback function.
| web/chat/thread-menu.react.js | ||
|---|---|---|
| 43 ↗ | (On Diff #9616) | It is safer to set the state using function instead of basing it on a current state. In this case it shouldn't matter that much, but it prevents an issue which might occur when state updates are grouped together. |
| 51 ↗ | (On Diff #9596) | There are a couple of options here. We can e.g. check event target property and check if the click was inside the menu, or we could render the menu in front of an invisible overlay that would handle the click event. A couple of options can be found here https://stackoverflow.com/questions/32553158/detect-click-outside-react-component But the question is if we really need it now. Currently we're fine when we close the menu after every click, so maybe we can keep the current approach. And when we need to keep the menu open after some interaction, we can search for an alternative approach. |
Comment Actions
This comment has been deleted.
| web/chat/thread-menu.css | ||
|---|---|---|
| 17 ↗ | (On Diff #9616) | visual: padding needs to match figma:  |
| web/chat/thread-menu.react.js | ||
| 23 ↗ | (On Diff #9616) | rather than display:none, do you think just toggling the component showing or hiding in JSX would be more straight forward, easier for the developer read? |
| 46 ↗ | (On Diff #9616) | why not just .map over menuItems? |
| 53 ↗ | (On Diff #9616) |  should be size 24 from figma |
| web/chat/thread-menu.css | ||
|---|---|---|
| 17 ↗ | (On Diff #9616) | The paddings you showed are not computed for the whole icon but instead for its inner part, the circle.  But there's the issue with these, as they do not include the padding that should've been included in the icon itself. The correct paddings can be seen here:  This issue probably explains why we've introduced this complicated process of removing the padding from icons. It looks like we were not able to see the correct paddings and found a workaround for it. Instead we should have checked how to get those from figma. So unless we have a really good reason to keep removing the paddings, we should stop doing that and start using our tools correctly. |
| web/chat/thread-menu.react.js | ||
|---|---|---|
| 46 ↗ | (On Diff #9616) | The idea here is to prevent displaying the whole menu button, when there are no items inside the menu. I'm not sure what you exactly mean by mapping over menuItems. |
| 53 ↗ | (On Diff #9616) | The icon is 24px in Figma, but in original version - containing paddings around. The size I used with our modified version of icon is more close to the original design. |
Comment Actions
There seems to be a strong disagreement on the subject of whether we should strip padding from icons. @benschac, @palys-swm, @def-au1t – I suspect the quickest / healthiest way to resolve it would be a video call. Would suggest scheduling that ASAP in order to unblock @def-au1t
| web/chat/thread-menu.css | ||
|---|---|---|
| 17 ↗ | (On Diff #9616) | Chatted with @palys-swm today about Icon sizes. @palys-swm is right. I shouldn't have removed the paddings from the Icon sets. Initially, when I looked at the Figma I didn't see that the padding from the icon set was included in the design. I'm going to re-import the icon set with the default paddings today. Sorry about the misunderstanding. |
Comment Actions
I need to update the Icon sets and use the default icons with their paddings.
https://linear.app/comm/issue/ENG-800/re-introduce-default-paddings-in-icon-set