Page MenuHomePhabricator

[web] Introduce add user item
ClosedPublic

Authored by tomek on Jun 14 2022, 12:27 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Jul 5, 6:13 PM
Unknown Object (File)
Tue, Jul 2, 4:11 PM
Unknown Object (File)
Sun, Jun 30, 8:58 AM
Unknown Object (File)
Sun, Jun 30, 1:52 AM
Unknown Object (File)
Wed, Jun 26, 11:22 PM
Unknown Object (File)
Tue, Jun 25, 10:34 AM
Unknown Object (File)
Sat, Jun 22, 4:04 AM
Unknown Object (File)
Sat, Jun 22, 4:04 AM

Details

Summary

This component will be used on friend and block add lists.

add_user_2.png (132×1 px, 10 KB)

Test Plan

Render the component on a modal and check if it looks correctly and reacts to user click action

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul added inline comments.
web/settings/relationship/add-users-list-item.react.js
16 ↗(On Diff #13505)

Maybe something like onAddUserButtonClicked/onAddUserClicked/etc?

Probably doesn't make a difference now since the component is pretty simple, but maybe in the future there are multiple interactive "clickable" elements?

23 ↗(On Diff #13505)

Not sure whether we want to capitalize "add" here?

I think we capitalize user actions in other places (eg Add Friend, Block User, etc) so maybe we should keep that consistent?


As an aside, this looks like a place where we could benefit from some design work? Maybe instead of "add" we could include an icon like the following:

Screen Shot 2022-06-16 at 10.39.40 AM.png (822×802 px, 42 KB)

?


None of these block the diff, just something we might want to consider.

This revision is now accepted and ready to land.Jun 16 2022, 7:41 AM
web/settings/relationship/add-users-list-item.react.js
23 ↗(On Diff #13505)

(this was the icon I intended to attach:

Screen Shot 2022-06-16 at 10.46.21 AM.png (824×808 px, 41 KB)

)

web/settings/relationship/add-users-list-item.react.js
16 ↗(On Diff #13505)

I don't think it is a good idea to complicate this - it is unlikely that this row would have a couple of clickable elements, and even if we decided to add one, we could then rename the callback.
Also, the whole component is a single button so adding button in the name might be confusing.

On the other hand, it would be beneficial to have less abstract name, describing what this action does instead of saying when it is used, so addUser might be a good idea.

23 ↗(On Diff #13505)

Yes, we should use capital A here. Regarding using icons, I'm not really sure - I created a task https://linear.app/comm/issue/ENG-1278/consider-using-icons-instead-of-text-in-member-user-add-modals where we can decide if this is a good idea.

Rename function and change button text

This revision was automatically updated to reflect the committed changes.