Page MenuHomePhabricator

[web] Introduce `Settings` and `Notifications` thread menu actions
ClosedPublic

Authored by jacek on Feb 14 2022, 3:37 AM.
Tags
None
Referenced Files
F3272192: D3189.diff
Sat, Nov 16, 5:30 AM
Unknown Object (File)
Thu, Nov 14, 8:50 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

Details

Summary

Introduced Settings and Notifications actions in thread menu.
Currently, there are no modals displayed after clicking the button. I'll add onCLick action in follow-up diff.

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

Test Plan

Check if items are displayed correctly in every thread.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek added inline comments.
web/chat/thread-menu.react.js
23–33 ↗(On Diff #9598)

These never change, so instead of using memo with empty dependencies, we could as well use refs.

36 ↗(On Diff #9598)

We've discussed it, but it might be a good idea to explain what happens here for other reviewers.
So basically, we will be adding menu items to items. Those items are rendered conditionally, so there will be null entries and we need to filter them out here.

This revision is now accepted and ready to land.Feb 14 2022, 4:25 AM
This revision now requires review to proceed.Feb 14 2022, 4:25 AM
web/chat/thread-menu.react.js
23–33 ↗(On Diff #9598)

I'm not sure if I fully understand what do you mean by "use refs"? Could you provide an example, please? Would it change any behavior?

Would prefer less boilerplate

web/chat/thread-menu.react.js
23–33 ↗(On Diff #9621)

These can just be defined outside the function, or instead the useMemo for menuItems

This revision is now accepted and ready to land.Feb 14 2022, 9:03 PM
web/chat/thread-menu.react.js
23–33 ↗(On Diff #9598)

The behavior would remain the same, but the code will be simpler.

const settingsItem = React.useRef(<ThreadMenuItem key="settings" text="Settings" icon={faCog} />);

And then later

const items = [settingsItem.current, notificatiosItem.current];

But Ashoat suggested even easier solution, with functions defined outside this component. We can do that, because they do not depend on props.

Moved components inside menuItems memo, as ThreadMenuItem component is now in React.memo itself.