Page MenuHomePhabricator
Differential D4305

[web] Prepare a list of users that should be displayed
ClosedPublic
Actions

Authored by tomek on Jun 21 2022, 5:14 AM.
Tags
None
Referenced Files
F2906802: D4305.diff
Sun, Oct 6, 10:01 AM
Unknown Object (File)
Wed, Sep 25, 3:07 AM
Unknown Object (File)
Tue, Sep 24, 5:50 PM
Unknown Object (File)
Mon, Sep 9, 7:04 PM
Unknown Object (File)
Mon, Sep 9, 7:04 PM
Unknown Object (File)
Mon, Sep 9, 7:04 PM
Unknown Object (File)
Mon, Sep 9, 7:04 PM
Unknown Object (File)
Mon, Sep 9, 7:01 PM
View All Files
Subscribers
• abosh
• adrian
ashoat

Details

Reviewers
• jacek
atul
Commits
rCOMM3fc1f94f82fb: [web] Prepare a list of users that should be displayed
Summary

Create a list of users and sort it by username.

Depends on D4269

Test Plan

Render the component and check if the list is sorted correctly.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

tomek created this revision.Jun 21 2022, 5:14 AM
Herald added subscribers: • abosh, • adrian, ashoat. · View Herald TranscriptJun 21 2022, 5:14 AM
tomek edited the summary of this revision. (Show Details)Jun 21 2022, 5:14 AM
tomek added a parent revision: D4269: [web] Merge store and server search results.
tomek added a child revision: D4306: [web] Add option to filter users by status.Jun 21 2022, 5:19 AM
Harbormaster completed remote builds in B9844: Diff 13619.Jun 21 2022, 5:19 AM
tomek requested review of this revision.Jun 21 2022, 5:19 AM
atul removed a reviewer: • benschac.Jun 21 2022, 9:54 AM
atul accepted this revision.Jun 21 2022, 10:12 AM

Looks good! Added alternative approach using .map(...).sort(...) as an inline comment, but feel free to land with either approach

web/settings/relationship/add-users-list.react.js
73–79 ↗(On Diff #13619)

Not sure if this is more/less readable, but I think we could do something like this as well:

return mergedUserInfos
  .map(({ userID }) => mergedUserInfos[userID])
  .sort((user1, user2) => user1.username.localeCompare(user2.username));
This revision is now accepted and ready to land.Jun 21 2022, 10:12 AM
tomek added inline comments.Jun 22 2022, 9:47 AM
web/settings/relationship/add-users-list.react.js
73–79 ↗(On Diff #13619)

Good catch - using functional approach is an improvement.

Your proposition needs to be modified, because mergedUserInfos is an object. We should use Object.keys(mergedUserInfos) instead.

tomek updated this revision to Diff 13671.Jun 22 2022, 9:49 AM

Use more functional approach

Harbormaster completed remote builds in B9882: Diff 13671.Jun 22 2022, 9:54 AM
Closed by commit rCOMM3fc1f94f82fb: [web] Prepare a list of users that should be displayed (authored by tomek). · Explain WhyJun 24 2022, 5:15 AM
This revision was automatically updated to reflect the committed changes.
tomek added a commit: rCOMM3fc1f94f82fb: [web] Prepare a list of users that should be displayed.

Revision Contents
Changeset List

PathSize
web/
settings/
relationship/
10 lines

Diff 13671

View Options

web/settings/relationship/add-users-list.react.js

Loading...