Page MenuHomePhabricator
Differential D3188

[web] Introduce `ThreadMenuItem`
ClosedPublic
Actions

Authored by • jacek on Feb 14 2022, 3:32 AM.
Tags
None
Referenced Files
F3270528: D3188.diff
Sat, Nov 16, 4:35 AM
Unknown Object (File)
Sat, Nov 2, 6:37 AM
Unknown Object (File)
Sat, Nov 2, 6:37 AM
Unknown Object (File)
Sat, Nov 2, 6:37 AM
Unknown Object (File)
Sat, Nov 2, 6:37 AM
Unknown Object (File)
Sat, Nov 2, 6:37 AM
Unknown Object (File)
Sat, Nov 2, 6:35 AM
Unknown Object (File)
Sat, Nov 2, 6:17 AM
View All Files
Subscribers
• adrian
ashoat
atul
• karol

Details

Reviewers
tomek
• benschac
atul
ashoat
Commits
rCOMMf443e034884a: [web] Introduce `ThreadMenuItem`
Summary

Introduced component for item inside thread menu.

Currently, it uses FontAwesome icons, which will be replaced in the future with SWMIconPack.
Because of quite complicated way of adding SWM icons to project, I think, it would be better to replace all icons at once in a follow-up task.

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

Test Plan

Tested after adding menu actions in next diffs.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• jacek created this revision.Feb 14 2022, 3:32 AM
Herald added subscribers: • karol, atul, • adrian, ashoat. · View Herald TranscriptFeb 14 2022, 3:32 AM
• jacek edited the summary of this revision. (Show Details)Feb 14 2022, 3:34 AM
Harbormaster completed remote builds in B6775: Diff 9597.Feb 14 2022, 3:37 AM
• jacek requested review of this revision.Feb 14 2022, 3:37 AM
• jacek added a child revision: D3189: [web] Introduce `Settings` and `Notifications` thread menu actions.Feb 14 2022, 3:38 AM
• jacek added a parent revision: D3187: [web] Introduce `ThreadMenu`.
• jacek added a child revision: D3192: [web] Introduce `Leave` thread menu action.Feb 14 2022, 3:53 AM
• jacek added a child revision: D3191: [web] Introduce `Subchannels` thread menu actions.
• jacek added a child revision: D3190: [web] Introduce `Members` and `Sidebars` thread menu actions.
• jacek added reviewers: • benschac, atul.
tomek accepted this revision.Feb 14 2022, 4:13 AM
tomek added inline comments.
web/chat/thread-menu-item.react.js
11 ↗(On Diff #9597)

If that's not a problem, we should prefer accepting all the functions, not just the ones that do not return

web/chat/thread-menu.css
34 ↗(On Diff #9597)

Why do we need to provide column gap when there's only one column?

This revision is now accepted and ready to land.Feb 14 2022, 4:13 AM
Herald added a reviewer: ashoat. · View Herald TranscriptFeb 14 2022, 4:13 AM
This revision now requires review to proceed.Feb 14 2022, 4:13 AM
• jacek added inline comments.Feb 14 2022, 5:30 AM
web/chat/thread-menu.css
34 ↗(On Diff #9597)

There are two columns - for icon and text

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

I guess we should use margins instead - and use column-gap when there are multiple columns of text.
But now I noticed that something is wrong here. Where is css.topBarMenuActionDescription defined?

This revision now requires changes to proceed.Feb 14 2022, 5:37 AM
• jacek added inline comments.Feb 14 2022, 6:09 AM
web/chat/thread-menu.css
34 ↗(On Diff #9597)

The class is not necessary now. I'll remove it.
@palys-swm what is the profit of using margins instead of column-gap here? II suppose it would work in the same way, isn't it?
How do we decide which element should contain margin - icon or text?

• jacek updated this revision to Diff 9617.Feb 14 2022, 6:39 AM

removed redundant css class, replaced column-gap with margin, changed onClick callback function type.

Harbormaster completed remote builds in B6792: Diff 9617.Feb 14 2022, 6:44 AM
tomek accepted this revision.Feb 14 2022, 7:18 AM
tomek added inline comments.
web/chat/thread-menu.css
34 ↗(On Diff #9597)

Margins are better understood by developers, but more importantly, all the browsers support them. With column-gap https://developer.mozilla.org/en-US/docs/Web/CSS/column-gap the are some issues, especially due to the fact that

Initially a part of Multi-column Layout, the definition of column-gap has been broadened to include multiple layout methods. Now specified in Box Alignment, it may be used in Multi-column, Flexible Box, and Grid layouts.

Regarding the placement of margins, I don't think it matters. I can see that you added it to the icon and I think we can keep it there.

ashoat requested changes to this revision.Feb 14 2022, 9:10 PM
ashoat added 1 blocking reviewer(s): • benschac.

@benschac should take a look here. I suspect he will have a couple comments, both about margins and about using the SWM icon pack instead of the FontAwesome ones

This revision now requires changes to proceed.Feb 14 2022, 9:11 PM
• jacek updated this revision to Diff 9714.Feb 16 2022, 4:52 AM

Added React.memo after Tomek's suggestion

Harbormaster completed remote builds in B6866: Diff 9714.Feb 16 2022, 4:57 AM
tomek accepted this revision.Feb 16 2022, 5:07 AM

@benschac could you take a look to unblock?

• benschac requested changes to this revision.Feb 16 2022, 7:05 AM
• benschac added inline comments.
web/chat/thread-menu-item.react.js
24 ↗(On Diff #9714)

we should create a variant in our button component rather than creating more one-off components.

button.react.js in the components folder.

web/chat/thread-menu.css
44–45 ↗(On Diff #9714)

I don't think we need to be explicitly setting width here.

This revision now requires changes to proceed.Feb 16 2022, 7:05 AM
• jacek added inline comments.Feb 21 2022, 4:05 AM
web/chat/thread-menu-item.react.js
24 ↗(On Diff #9714)

I think it shouldn't be a part of Button component, the only thing common for other buttons and this menu item is usage of semantic html <button> tag, which could be also li tag as well. I think our Button component should be reserved for more for detached-like buttons and not used for elements of the menu list.

• jacek updated this revision to Diff 9795.Feb 21 2022, 4:06 AM

Updated CSS

• jacek added inline comments.Feb 21 2022, 4:08 AM
web/chat/thread-menu.css
44–45 ↗(On Diff #9714)

It is now required in order to properly align text, but will be removed in the future after switching to SWMIconPack (where all icons have the same width)

Harbormaster completed remote builds in B6941: Diff 9795.Feb 21 2022, 4:11 AM
ashoat added a comment.Feb 21 2022, 10:51 PM

@benschac, in your review please make sure you address any potential concerns relating to the two things I mentioned below:

In D3188#85300, @ashoat wrote:

@benschac should take a look here. I suspect he will have a couple comments, both about margins and about using the SWM icon pack instead of the FontAwesome ones

ashoat added inline comments.Feb 21 2022, 10:53 PM
web/chat/thread-menu-item.react.js
24 ↗(On Diff #9714)

Agree with @def-au1t here... not sure I understand why these two types of HTML <button>s need to be represented by the same Button component

ashoat added a comment.Feb 21 2022, 10:54 PM
This comment was removed by ashoat.
ashoat requested changes to this revision.EditedFeb 22 2022, 12:21 AM

Just noticed that the FontAwesome vs. SWM icon question is referenced in the diff description:

Currently, it uses FontAwesome icons, which will be replaced in the future with SWMIconPack.
Because of quite complicated way of adding SWM icons to project, I think, it would be better to replace all icons at once in a follow-up task.

@def-au1t, can you link the follow-up task (either on Linear or Phabricator)? You should never reference a follow-up task without linking where that task is tracked.

Once you share that link and @benschac responds, this diff should be unblocked. I think the discussion here is tied to the discussion that needs to happen on D3187, so let's make sure you guys cover this when you have that discussion.

This revision now requires changes to proceed.Feb 22 2022, 12:21 AM
• jacek added a comment.Feb 22 2022, 5:05 AM
In D3188#86963, @ashoat wrote:

@def-au1t, can you link the follow-up task (either on Linear or Phabricator)? You should never reference a follow-up task without linking where that task is tracked.

Link to follow-up task on Linear: https://linear.app/comm/issue/ENG-700/replace-fontawesome-icons-with-swmiconpack

ashoat accepted this revision.Feb 22 2022, 12:48 PM

Link to follow-up task on Linear: https://linear.app/comm/issue/ENG-700/replace-fontawesome-icons-with-swmiconpack

You forgot to subscribe people to that task! Please always subscribe people to tasks you create. (I added some folks)

This revision now requires review to proceed.Feb 22 2022, 12:48 PM
• benschac accepted this revision.Feb 23 2022, 4:06 AM
• benschac added inline comments.
web/chat/thread-menu-item.react.js
24 ↗(On Diff #9714)

Base CSS for the button is the same across the board:

border: none;
cursor: pointer;
background-color: transparent;

This button could be called something like transparent and used again.

This revision is now accepted and ready to land.Feb 23 2022, 4:06 AM
• benschac added inline comments.Feb 23 2022, 4:09 AM
web/chat/thread-menu.css
34 ↗(On Diff #9597)

I'm not a fan of margin: https://mxstbr.com/thoughts/margin/ for these reasons.

I'm also late to this diff and I don't think it makes much difference.

tomek added inline comments.Feb 23 2022, 4:27 AM
web/chat/thread-menu.css
34 ↗(On Diff #9597)

Thanks for sharing this - it really makes sense!
In this case I would prefer using flexbox with space-between instead of using column-gap

Closed by commit rCOMMf443e034884a: [web] Introduce `ThreadMenuItem` (authored by • jacek). · Explain WhyMar 1 2022, 2:13 AM
This revision was automatically updated to reflect the committed changes.
• jacek added a commit: rCOMMf443e034884a: [web] Introduce `ThreadMenuItem`.

Revision Contents
Changeset List

PathSize
web/
chat/
37 lines
33 lines
5 lines

Diff 9795

View Options

web/chat/thread-menu-item.react.js

Loading...
View Options

web/chat/thread-menu.css

Loading...
View Options

web/theme.css

Loading...