[web] Introduce user list component
ClosedPublic
Actions

Authored by tomek on Apr 8 2022, 7:17 AM.

Details

Reviewers

• benschac
atul
• jacek

Commits

rCOMMdc781717c0b5: [web] Introduce user list component

Summary

This component is responsible for selecting, filtering and sorting users. It also renders user rows. Its behavior is configurable by providing functions and component that is used for rendering. It can be used to render both friend and block list.

Providing row component is similar to how e.g. FlatList from RN is used so it should be a familiar pattern. Receiving filter ond compare function allows sharing the biggest part of logic between friend and block list.

Depends on D3644

Test Plan

Provide necessary functions and component and check if users were selected properly and rendered in the correct order.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

tomek created this revision.Apr 8 2022, 7:17 AM

Herald added subscribers: • abosh, • karol, • adrian, ashoat. · View Herald TranscriptApr 8 2022, 7:17 AM

Harbormaster completed remote builds in B8028: Diff 11241.Apr 8 2022, 7:22 AM

tomek requested review of this revision.Apr 8 2022, 7:22 AM

tomek added a child revision: D3675: [web] Introduce user list modal.Apr 8 2022, 7:23 AM

Looks good!

web/settings/relationship/user-list.react.js
15 ↗	(On Diff #11241)	Since we're exporting another "props" type above (`UserRowProps`), should we maybe name this type `UserListProps` to make the distinction a bit more clear?
18 ↗	(On Diff #11241)	nit: wonder if we can avoid including "Function" in the function name with something like `userComparator` or `usersComparator`
22 ↗	(On Diff #11241)	nit: personally think `searchQuery` might be a better name than `searchText` but don't feel too strongly feel free to ignore this one
25 ↗	(On Diff #11241)	The pattern we seem to follow in other components is to append something like export default UserList; to the end of the file. We should probably follow that convention here, but let me know if I'm missing something
40–59 ↗	(On Diff #11241)	I wonder if we could simplify this logic a bit like this? Not sure it actually makes anything more readable though

This revision is now accepted and ready to land.Apr 10 2022, 4:03 PM

tomek added inline comments.Apr 12 2022, 10:13 AM

web/settings/relationship/user-list.react.js
15 ↗	(On Diff #11241)	Ok, that makes sense
18 ↗	(On Diff #11241)	Ok
22 ↗	(On Diff #11241)	Agree, it is slightly better, but we're using `searchText` in a lot of different places and I don't want to introduce inconsistencies - it might be confusing what is the difference between `searchQuery` and `searchText`. I also don't think that changing it in all the places is worth it. So I'd like to keep the current name.
25 ↗	(On Diff #11241)	The missing part is that we're avoiding mixing default and named export. But I can move the imports to the bottom to make it more consistent with the rest of the codebase.
40–59 ↗	(On Diff #11241)	I like having more functional approach here - going to make this change. We can even replace `for` with a combination of `filter` and `map` - will give this approach a try.

atul added inline comments.Apr 12 2022, 10:50 AM

web/settings/relationship/user-list.react.js
22 ↗	(On Diff #11241)	Ah gotcha, `searchText` definitely makes more sense then

tomek marked 4 inline comments as done.Apr 14 2022, 5:18 AM

tomek added inline comments.

web/settings/relationship/user-list.react.js
25 ↗	(On Diff #11241)	Decided to keep the current approach as `export { type UserRowProps, UserList }` and `export { UserRowProps, UserList }` are invalid (`UserRowProps` is a type)

Updates after review

Harbormaster completed remote builds in B8204: Diff 11475.Apr 14 2022, 7:24 AM

Closed by commit rCOMMdc781717c0b5: [web] Introduce user list component (authored by tomek). · Explain WhyApr 15 2022, 2:16 AM

This revision was automatically updated to reflect the committed changes.

tomek added a commit: rCOMMdc781717c0b5: [web] Introduce user list component.

ashoat added inline comments.Apr 18 2022, 7:48 PM

web/settings/relationship/user-list.react.js
22	Today in the codebase there is a convention to avoid inline `export` statements like this, except for `export default` and in files such as those in `lib/types`. Instead, we tend to prefer having a single `export` statement at the bottom of the file. Open to changing this convention, but would prefer to do it as a discussion instead of gradually introducing nonconforming code