Show a list of users that are about to be added. Clicking an x button next to their name removes them from the list.
Depends on D4308
Details
Details
- Reviewers
• jacek atul - Commits
- rCOMM64fe033c7e30: [web] Display user tags
Add a couple of users and then remove a couple of them. Every time a name is clicked on the list a tag is added. Every time a tag is closed, the name is added back to the list.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
Looks good, two questions inline to be addressed before landing
| web/settings/relationship/add-users-list.react.js | ||
|---|---|---|
| 117–123 ↗ | (On Diff #13624) | Could we simplify this and replace with: return pendingUsers.filter(({id}) => id !== userID)? I'm guessing the reason we're checking if filteredPendingUsers is the same length as pendingUsers is to avoid setting pendingUsersToAdd to a new identical object if nothing has changed... But, can't we assume that deselectUser will result in a different pendingUsersToAdd? If there is some edge case where that isn't true, is the cost of creating the "same" object with a different reference a big deal? |
| 137 ↗ | (On Diff #13624) | Might be missing something here, but shouldn't we memoize this with React.useCallback(...)? Maybe deselectUser should accept a userInfo instead of userID if that's simpler? |
Comment Actions
Giving names to the hooks which are used might indeed improve the readability. On the other hand, moving the hooks to other place would make the code more scattered, which could make it harder to understand. Will explore it a bit.
| web/settings/relationship/add-users-list.react.js | ||
|---|---|---|
| 117–123 ↗ | (On Diff #13624) | Ok, we can do that - it was probably over-engineered. |
| 137 ↗ | (On Diff #13624) | This is enclosed by a memo, so memoizing it doesn't make a lot of difference.
I don't see why it would be simpler. Which part would be simplified? |
Comment Actions
After some thought, I don't think extracting these is beneficial. All the values returned by hooks have meaningful names, so the purpose is well-documented. Creating separate hooks would make this code more scattered - it doesn't make too much sense because this is the only place where these will ever be used.