Page MenuHomePhabricator

[web] Introduce `AddMembersItem` component
ClosedPublic

Authored by jacek on Apr 15 2022, 3:57 AM.
Tags
None
Referenced Files
F3177389: D3739.diff
Thu, Nov 7, 11:16 PM
Unknown Object (File)
Tue, Nov 5, 4:23 AM
Unknown Object (File)
Wed, Oct 23, 2:59 AM
Unknown Object (File)
Sun, Oct 20, 7:21 PM
Unknown Object (File)
Sun, Oct 20, 7:20 PM
Unknown Object (File)
Sun, Oct 20, 7:20 PM
Unknown Object (File)
Thu, Oct 10, 11:00 AM
Unknown Object (File)
Wed, Oct 9, 7:07 PM

Details

Summary

Introduce component rendering single user on the list in AddMembersModal. Because I shared some common logic with native, user information are passed in UserListItem type.

GIF presenting the items:

Screenshot_Google Chrome_2022-04-15_130227.gif (334×442 px, 336 KB)

Test Plan

Component can be tested after introducing AddMembersModal

Diff Detail

Repository
rCOMM Comm
Branch
jacek/add-m-1
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

tomek requested changes to this revision.Apr 15 2022, 9:03 AM
tomek added inline comments.
web/modals/threads/members/add-members-item.react.js
19

Could you explain what happens here? Why userInfo.alertTitle === undefined means that user can be added?

28–30

The returned value will be different after each render, so we should probably memoize it

44

We should prefer button - it's more semantic. We can also set disabled attribute when !canBeAdded

45–46

Why do we need two separate classes for them?

This revision now requires changes to proceed.Apr 15 2022, 9:03 AM
jacek added inline comments.
web/modals/threads/members/add-members-item.react.js
19

It requires some explanation, because it's a bit strange. This is how it's currently implemented in React Native.
In the mobile app, the callback that adds user is not executed if UserListItem have alertText set:
https://github.com/CommE2E/comm/blob/master/native/components/user-list-user.react.js#L57-L60

These values are set in:
https://github.com/CommE2E/comm/blob/master/lib/shared/search-utils.js#L113-L137

In fact, I should have checked alertText instead alertTitle, so I'll correct it, although both these fields are filled together and it also works correctly now.

28–30

Are you sure it will? I did a quick test, and it seems that the value doesn't change between renders.
Maybe the classNames library has some optimizations?

44

I think the best semantic matching tag would be here li as it's more an element of the list (although is clickable).

45–46

I think it can even be simplified by using, .addMemberItem > div selector in CSS. What do you think? Or should I leave a separate class name for the items in such case?

Updates following review comments

tomek requested changes to this revision.Apr 25 2022, 6:28 AM
tomek added inline comments.
web/modals/threads/members/add-members-item.react.js
18 ↗(On Diff #11776)

In fact, I should have checked alertText instead alertTitle, so I'll correct it, although both these fields are filled together and it also works correctly now.

You're still using alertTitle. Also, I think it will be safer to use !userInfo.alertText.

28–30

It's better to check their source code. But even if their code does a memoization, it is safer for us to do the same, just to avoid issues when upgrading the library.

It doesn't matter any more, as we're no longer using it here.

44

If we really want to be correct, we should probably wrap button with li. But for now button sounds like a good enough solution.

45–46

It's more maintainable to use classes - we avoid accidental changes. But at this point, it looks like .addMemberName and .addMemberAction are used but not defined. We can e.g. introduce a new class .label and use it instead of button.addMemberItem > div.

This revision now requires changes to proceed.Apr 25 2022, 6:28 AM

replace alertTitle with alertText and change css class name to label

web/modals/threads/members/add-members-item.react.js
18 ↗(On Diff #11776)

Sorry for missing it

This revision is now accepted and ready to land.Apr 25 2022, 6:58 AM