Page MenuHomePhabricator

[web] Introduce `ThreadMembersList` component
ClosedPublic

Authored by jacek on Mar 9 2022, 5:33 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Oct 1, 1:37 AM
Unknown Object (File)
Mon, Sep 23, 8:50 AM
Unknown Object (File)
Mon, Sep 23, 8:50 AM
Unknown Object (File)
Mon, Sep 23, 8:49 AM
Unknown Object (File)
Mon, Sep 23, 8:49 AM
Unknown Object (File)
Mon, Sep 23, 8:49 AM
Unknown Object (File)
Mon, Sep 23, 8:49 AM
Unknown Object (File)
Sat, Sep 21, 6:43 AM

Details

Summary

Create component with list of thread members grouped by first letter of username. The component will be displayed in modal.
The component disallows list scrolling if any menu from its children is open (that's why it keeps opened menus list).

Screenshot_Google Chrome_2022-03-09_144729.png (327×339 px, 14 KB)

Test Plan

The component can be tested after introducing members modal.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

tomek requested changes to this revision.Mar 14 2022, 10:05 AM
tomek added inline comments.
web/modals/threads/members/members-list.react.js
4 ↗(On Diff #10205)

My preference is to use import _ from 'lodash' but in most other places we're importing just a single function from fp package, so to keep consistency use e.g. import _groupBy from 'lodash/fp/groupBy' and we can discuss which approach is better for us.

23 ↗(On Diff #10205)

Why do we need to have an array here? Is there a reason why we can't have a state with single nullable number?

41–47 ↗(On Diff #10205)

To make the memoization work here we should give ThreadMember its user.id and setOpenMenus function and then create callback there

51 ↗(On Diff #10205)

It's a header so we should probably use h here

This revision now requires changes to proceed.Mar 14 2022, 10:05 AM
web/modals/threads/members/members-list.react.js
23 ↗(On Diff #10205)

When I implemented it with single number, there was a race condition when I was switching between menus - when trying to open one if the other is opened. I happened, that the second one was opened a bit quicker that the first was closed, and the state became invalid. The solution with array of opened menus prevents the problems here.
An alternative approach would be to block opening a second menu if one is opened, but I think the current (without blocking opening menus) is more user-friendly (as user don't have to close one menu to open another).
I'm curious @palys-swm what do you think is the best approach here.

web/modals/threads/members/members-list.react.js
23 ↗(On Diff #10205)

I'm not sure why this race condition occurred. But it is possible that onMenuChange can be implemented in a way that avoids this issue:
So first of all, we would have a state const [openMenu, setOpenMenu] = React.useState<?number>(null)
When we open a menu, we can just set it to the new menu id.
Then, when we close a menu, we should set it to null only when user.id matches. So in other words, closing a menu should set a state only when this menu was currently opened.
Could that solve the issue?

tomek added inline comments.
web/modals/threads/members/members-list.react.js
25

Maybe hasMembers?

37–45

Do we need to sort here by username?

This revision is now accepted and ready to land.Mar 17 2022, 3:53 AM
web/modals/threads/members/members-list.react.js
35

This shouldn't happen, but if somebody's username is the empty string, a[0].localCompare could end up with a fatal error (dereferencing localCompare on undefined). Probably not worth worrying about (the username regex should prevent the empty string from working)

web/modals/threads/members/members-list.react.js
37–45

Yes, good catch. The users are currently displayed sorted by ID by default. I didn't notice it because all users I had in database were registered in alphabetical order accidentally.