Paths

Table of Contentst

[web] introduce previouslySelectedUsers map to add users list
ClosedPublic
Actions

Authored by ginsu on Jan 25 2024, 12:01 PM.

Details

Reviewers

atul
inka

Commits

rCOMMa59a2f0b08d3: [web] introduce previouslySelectedUsers map to add users list

Summary

PLEASE NOTE THAT THIS DIFF AND SUBSEQUENT DIFFS IN THIS STACK WILL NOT BE LANDED UNTIL MORE OF THE REDESIGN IS READY SINCE THIS WILL CAUSE REGRESSIONS IN PROD

This diff introduces the previouslySelectedUsers state. Based on the design requirements in the figma we only want to move users in and out of the previously section whenever the modal view changes into "search mode" (whenever searchModeActive changes).

We should not jump the members on their own, they will only be moved when the modal “changes” views when a user searches and then clears.

Screenshot 2024-01-25 at 2.51.14 PM.png (1×2 px, 441 KB)

Screenshot 2024-01-25 at 3.10.39 PM.png (1×1 px, 425 KB)

Subsequent diffs will handle introducing the previously selected users to the UI.

Linear task: https://linear.app/comm/issue/ENG-5960/selected-users-section

Depends on D10819

Test Plan

Please see the demo video below

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ginsu created this revision.Jan 25 2024, 12:01 PM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJan 25 2024, 12:01 PM

ginsu edited the summary of this revision. (Show Details)Jan 25 2024, 12:13 PM

ginsu edited the test plan for this revision. (Show Details)

ginsu added reviewers: atul, inka.

ginsu added a parent revision: D10819: [web] convert pendingUsersToAdd into a map.

ginsu attached a referenced file: F1088708: Screen Recording 2024-01-25 at 3.12.43 PM.mov. (Show Details)

ginsu attached a referenced file: F1088704: Screenshot 2024-01-25 at 2.51.14 PM.png. (Show Details)

ginsu added inline comments.Jan 25 2024, 12:17 PM

web/settings/relationship/add-users-list.react.js
89 ↗	(On Diff #36140)	Left a comment in the codebase to explain why we need this line, but based off the figma designs we only show vip users when the search query is empty and we only move users into the vip users section whenever we enter or leave the "search mode"

Harbormaster completed remote builds in B26163: Diff 36140.Jan 25 2024, 12:19 PM

ginsu requested review of this revision.Jan 25 2024, 12:19 PM

ginsu added a child revision: D10822: [web] introduce previouslySelectedUsers ui to add users list.Jan 25 2024, 12:34 PM

ginsu edited the summary of this revision. (Show Details)Jan 25 2024, 12:37 PM

What does "vip" mean in this situation? Not seeing anything in the linked Linear issue.

Let me know if I'm missing something

What does "vip" mean in this situation?

Very important person

In D10819 I wrote the following:

This next part of the stack will introduce the concept of "vip users" (happy to change the name of this just couldn't think of anything better). "vip users" will be selected users that are visually separated from the other users (similar to Linear).

Sorry probably should have put that at the top of this diff summary too

Left two comments, but also assuming that the video in the Test Plan is unfinished? It seems unusual that the selected users don't appear in the modal at all after they've been selected?

Edit: Test Plan in D10822 makes more sense.

web/settings/relationship/add-users-list.react.js
86 ↗	(On Diff #36140)	We typically capitalize all letter of acronyms (eg ID, URL, URI, etc) in variable names
88–89 ↗	(On Diff #36140)	Not fully understanding how the situation where "the search query is empty" is handled? Let me know if I'm missing something but this seems to track `searchText` rather than whether or not we're in "search mode"?

This revision now requires changes to proceed.Jan 29 2024, 7:33 PM

address comments

ginsu retitled this revision from [web] introduce vipPendingUsers map to add users list to [web] introduce previouslySelectedUsers map to add users list.Jan 31 2024, 2:46 PM

ginsu edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B26386: Diff 36481.Jan 31 2024, 2:57 PM

Thanks for addressing feedback!

This revision is now accepted and ready to land.Feb 5 2024, 12:13 PM

Actually, requesting changes. I think we can avoid using setState in useEffect

web/settings/relationship/add-users-list.react.js
86–91 ↗	(On Diff #36481)	Actually, I don't think we want to call `setState` from within `useEffect`.

This revision now requires changes to proceed.Feb 5 2024, 12:15 PM

atul added inline comments.Feb 5 2024, 12:31 PM

web/settings/relationship/add-users-list.react.js
86–91 ↗	(On Diff #36481)	Could we do something like const previouslySelectedUsers = React.useMemo( () => pendingUsersToAdd, [searchModeActive], ); ? I think that would be equivalent?

rerequesting review after discussing with @atul IRL

web/settings/relationship/add-users-list.react.js
86–91 ↗	(On Diff #36481)	Spoke to @atul about this IRL, but in D10937 we lift this piece of state into a provider so we still need to call `setState` from within `useEffect`.