Page MenuHomePhabricator

[web] Introduce common `Menu` component
ClosedPublic

Authored by jacek on Mar 9 2022, 3:33 AM.
Tags
None
Referenced Files
F3487166: D3374.id10187.diff
Wed, Dec 18, 7:14 AM
F3487146: D3374.id10517.diff
Wed, Dec 18, 7:04 AM
F3486590: D3374.diff
Wed, Dec 18, 5:17 AM
Unknown Object (File)
Wed, Nov 20, 4:30 PM
Unknown Object (File)
Wed, Nov 20, 4:30 PM
Unknown Object (File)
Wed, Nov 20, 4:30 PM
Unknown Object (File)
Wed, Nov 20, 4:30 PM
Unknown Object (File)
Wed, Nov 20, 4:30 PM

Details

Summary

Excluded threadMenu logc into separate Menu component. Changes names from ThreadMenu... to Menu....
The goal is to use the component in different places where menu with actions is needed.

Test Plan

Run web app, thread actions menu in top right corner should still look and behave in the same way.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek requested changes to this revision.Mar 10 2022, 3:52 AM
tomek added inline comments.
web/chat/thread-menu.react.js
182 ↗(On Diff #10187)

It looks like the style of this element is specific to thread-menu so it might be a better idea to keep thread-menu.css with only separator class

This revision now requires changes to proceed.Mar 10 2022, 3:52 AM

keep separator class in thread-menu.css

tomek requested changes to this revision.Mar 15 2022, 8:28 AM
tomek added inline comments.
web/components/menu.react.js
8–9 ↗(On Diff #10353)

Readonly

9 ↗(On Diff #10353)

Why do we need to use an array here? I think that children?: React.Node might work fine. In that case, we can use React.Children.count to get a number of children

This revision now requires changes to proceed.Mar 15 2022, 8:28 AM

Changes after following comments. Rebase.

Made prop "children" optional

This revision is now accepted and ready to land.Mar 17 2022, 2:40 AM
ashoat requested changes to this revision.Mar 17 2022, 11:47 AM
ashoat added inline comments.
web/components/menu.react.js
18 ↗(On Diff #10433)

I've never had to do something like this, so I'm not sure what the best pattern is. This approach strikes me as imperative rather than declarative, and React usually tries to be declarative. But I guess maybe there's no declarative way to do this in React (add a listener to document).

It looks like you're handling the lifecycle well. If you have any resources you can link that talk about best practices for this sort of thing in React, that would be great!

36 ↗(On Diff #10433)

React.Children.count is a function that takes a parameter

This revision now requires changes to proceed.Mar 17 2022, 11:47 AM

Add missing parameter in React.Children

This revision is now accepted and ready to land.Mar 20 2022, 8:59 PM
This revision was automatically updated to reflect the committed changes.