Page MenuHomePhabricator

[web] Introduce `ComposeSubchannel` modal
ClosedPublic

Authored by jakub on Sep 20 2022, 1:44 AM.
Tags
None
Referenced Files
F3374090: D5187.diff
Tue, Nov 26, 1:44 PM
Unknown Object (File)
Sat, Nov 23, 1:19 AM
Unknown Object (File)
Sat, Nov 23, 1:04 AM
Unknown Object (File)
Sat, Nov 23, 12:43 AM
Unknown Object (File)
Sat, Nov 23, 12:35 AM
Unknown Object (File)
Fri, Nov 15, 2:22 AM
Unknown Object (File)
Sat, Nov 9, 1:46 PM
Unknown Object (File)
Sat, Nov 9, 12:58 PM

Details

Summary

Depends on D5186
Context: here

Introduce modal for creating subchannels on web.
We would like to allow users to compose community subchannels on the web.

This diff contains complete user interface for creating all 4 allowed types of subchannels (open/secret; standard/announcement).

In this component, we use Stepper first time. This modal contains 2 pages/steps:

  • SubchannelSettings, which is intended for changing subchannel configuration

image.png (1×836 px, 118 KB)

  • SubchannelMemebers, which contains potential members list, last step

image.png (1×848 px, 102 KB)

Test Plan

a) Opening ComposeSubchannelModal

  1. Open thread menu
  2. Click on Create new subchannel

ComposeSubchannelModal appears.

b) Using form:

First step - SubchannelSettings:

  1. Enter a new subchannel name. Until the textbox is empty, you couldn't go to next step
  2. Choose channel type. By default, it is set to COMMUNITY_OPEN_SUBTHREAD

Second step - SubchannelMembers:

  1. Choose at least one member
  2. Click Create:
  3. Create button shows loading indicator:
    • if success -> form disappear and currently active thread changes to the newly created subchannel.
    • else -> enable Create button and show error message

Go back to previous page/step:

  1. When you are on SubchannelMembers, click on Back button
  2. Stepper changes current active step to settings.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
web/modals/threads/create/compose-subchannel-modal.react.js
186–192 ↗(On Diff #16927)

There's also no benefit to memoizing a string (unless there was some intense computation required to determine the string). The benefits of memoization come into play with objects (including arrays, functions, etc) and maintaining referential equality.

Found the following resource extremely helpful, might be worth a read: https://blog.isquaredsoftware.com/2020/05/blogged-answers-a-mostly-complete-guide-to-react-rendering-behavior/

187 ↗(On Diff #16927)

Can we just make this check activeStep === 'members'?

195 ↗(On Diff #16927)

We're pulling parentThreadInfo out of props on line 38, can we pull out onClose there as well?

web/modals/threads/create/steps/subchannel-members-list.react.js
57 ↗(On Diff #16927)

Do we not want to include the user.id !== currentUserId check from above here as well?

web/modals/threads/create/steps/subchannel-settings.react.js
14 ↗(On Diff #16927)

Should this be plural or just Visibility?

16–24 ↗(On Diff #16927)

Typically we define the Props type for a component right before the component is defined. Eg in this case we'll probably want to move Props to around line 49.

This revision now requires changes to proceed.Sep 26 2022, 12:39 PM
web/modals/threads/create/compose-subchannel-modal.react.js
130 ↗(On Diff #16927)

Why are we using .trim() here? It seems like it could break some equality assumptions to modify channelName like that?

143 ↗(On Diff #16927)

We usually don't leave TODO comments in the codebase.

I think you also have an extra slash at the start of the comment (/// vs //)

web/modals/threads/create/steps/subchannel-settings.react.js
1 ↗(On Diff #16927)

Elsewhere in the codebase we have a newline after the // @flow annotation

14 ↗(On Diff #16927)

You refer to the same type as VisibilityType in compose-subchannel-modal.react, would be good to match that for consistency

72 ↗(On Diff #16927)

I believe we can use the megaphone icon in the CommIcons component here instead

atul added reviewers: ginsu, rohan. atul added 1 blocking reviewer(s): tomek.Sep 26 2022, 12:54 PM
atul added a reviewer: abosh.
tomek requested changes to this revision.Sep 27 2022, 2:12 AM
tomek added inline comments.
web/modals/threads/create/compose-subchannel-modal.css
4 ↗(On Diff #16927)

We should use a variable for this - it is defined in typography.css

6–7 ↗(On Diff #16927)

We should avoid using colors directly and instead create new variables referencing them at the bottom of theme.css.

21–31 ↗(On Diff #16927)

Do we need to set max-height on container when we already have it on stepContainer?

web/modals/threads/create/compose-subchannel-modal.react.js
14 ↗(On Diff #16927)

In callback return value mixed is preferred because it allows using any function instead of only the ones that return nothing.

48 ↗(On Diff #16927)

Not really sure, but probably using $ReadOnlySet would make selectedUsers readonly. Can you check that?

170–184 ↗(On Diff #16927)

I think we can just create the steps with elements set from the beggining

188 ↗(On Diff #16927)

Why we're using exactly 11 here?

web/modals/threads/create/steps/subchannel-members-list.react.js
30–44 ↗(On Diff #16927)

This might be really confusing. In this component we handle two thread infos: one for parent and one for community. To determine parentMemberList we use communityMembers which is really confusing because a reader would expect that they are taken from parentThreadInfo. Maybe this is an intention and only the naming should be improved, but it might also be the case that the logic is invalid.

68–69 ↗(On Diff #16927)

Why do we use a magic number here? A lot better approach would be to have the whole text here and UI could determine the length using e.g. text-overflow css

web/modals/threads/create/steps/subchannel-members.css
7 ↗(On Diff #16927)

Is there a good reason for using sticky? Can't it be just always at the top? What are the benefits of using sticky here?

web/modals/threads/create/steps/subchannel-members.react.js
33–37 ↗(On Diff #16927)

What is the purpose of these lines? As far as I can tell, we have two thread infos: ancestorThreads[0] and parentThreadInfo, we take an id from one of them and use it to select a thread info from redux state. Can't we just use thread info that we had?

web/modals/threads/create/steps/subchannel-settings.react.js
14 ↗(On Diff #16927)

Or even better, define it in one place and use in the other

86 ↗(On Diff #16927)

Why do we have two empty spaces here?

jakub marked 27 inline comments as done.

Adjust to code review:

  • change flag icon to megaphone,
  • remove unnecessary whitespaces,
  • simplify code,
  • memoize steps,
  • use css variables from typography.css and theme.css,
  • adjust userInfos in MembersList to new type of AddMembersList
web/modals/threads/create/compose-subchannel-modal.react.js
14 ↗(On Diff #16927)

I had to use void type, because onClose props in Modal component requires it. Otherwise, flow throws this error:

Cannot create `Modal` element because  mixed [1] is incompatible with  undefined [2] in the return value of property `onClose`
48 ↗(On Diff #16927)

Sure, I check that and it could be $ReadOnlySet. I set this temporary and I missed it. I will change that

53–62 ↗(On Diff #16927)

I prepared this function at the beginning of working on this component. At that moment I used it for changing channel type in two another functions, but finally I use another approach.

So, we can move it inside onChangeChannelName

86 ↗(On Diff #16927)

I tried to keep conventions like in AddMembersList component, but I could change it.

123–149 ↗(On Diff #16927)

This object contains props that are provided to Stepper component (D5186) action buttons.
I gathered it in one place to avoid using useCallback in useMemo and to have navigation of modal in one place.

130 ↗(On Diff #16927)

I tried to remove unnecessary whitespaces in channelName, because it could allow us to create a channel with whitespaces-only name.
Alternatively, we could trim it when submitting the form.

What equality assumptions do you mean?

143 ↗(On Diff #16927)

Sure, I left this comment here as an interface. This function is already implemented in the next diff.

188 ↗(On Diff #16927)

This is tested value, that allow us to avoid resizing of the whole modal. We could also use better solution, using text-overflow: ellipsis;, but this approach might require a separate diff/task with Modal modifications.

web/modals/threads/create/steps/subchannel-members-list.react.js
30–44 ↗(On Diff #16927)

Members from parentThreadInfo are also community members, but we could also use parenThreadInfo.members inside parentMembersList instead of communityMembers.
Logic works fine, but I can code it better.

57 ↗(On Diff #16927)

currentUserId is currently included in parentMembers set. For better code maintenance, I could include this line.

68–69 ↗(On Diff #16927)

Sure, this approach requires modifications in AddMemberGroup css. For now, I could remove it and create separate diff for that.

web/modals/threads/create/steps/subchannel-members.react.js
33–37 ↗(On Diff #16927)

I tried to fix some flow type problems with that, but I check it now and it seems to work correctly without this workaround.

tomek requested changes to this revision.Sep 28 2022, 9:39 AM
tomek added inline comments.
web/modals/threads/create/compose-subchannel-modal.css
24–25

This doesn't make too much sense. In bigger windows, both height and max-height would be just 100vh. On smaller ones, the height will remain 100vh while max-height will be smaller than this - 533px. Probably something is wrong here.

web/modals/threads/create/compose-subchannel-modal.react.js
188 ↗(On Diff #16927)

Yeah, it makes sense to not complicate this diff with those changes - it might be a good followup diff / task.

web/modals/threads/create/steps/subchannel-members-list.react.js
51

When the username can be an empty string? How the app will behave in such a scenario? What the UI will look like?

I guess it might be better to not show empty names at all, but that depends on the possible reason.

80

All users in <not specified> doesn't sounds like a useful info for a user. When communityName can be null? Can we find a better message, e.g. All users in community?

68–69 ↗(On Diff #16927)

Makes sense

web/modals/threads/create/steps/subchannel-settings.css
2–7

We should introduce new variables / use existing ones (like in compose-subchannel-modal.css)

This revision now requires changes to proceed.Sep 28 2022, 9:39 AM
jakub marked 3 inline comments as done.

Adjust to code review:

  • fix stepContainer height
  • change <not specified to community
  • filter empty-username users from list
  • add new css variables
web/modals/threads/create/steps/subchannel-members-list.react.js
51

It's a workaround for flow issues. In our case, we should always have non-empty usernames. I tried to convert RelativeMemberInfo to UserListItem. I will filter it by username for making sure.

tomek requested changes to this revision.Sep 29 2022, 6:40 AM
tomek added inline comments.
web/modals/threads/create/compose-subchannel-modal.react.js
160–172 ↗(On Diff #17188)

The steps are always the same so there isn't too much value in having them in array. (A slight advantage is that the children of Stepper.Container don't change on render.) I think we should directly render the items instead of creating an array, memoizing it and then rendering.

web/modals/threads/create/steps/subchannel-members-list.react.js
51

Instead of replacing null with empty string, you should use stringForUser function

web/modals/threads/create/steps/subchannel-members.css
7 ↗(On Diff #16927)

The sticky position is still used... why do we need it?

This revision now requires changes to proceed.Sep 29 2022, 6:40 AM
jakub marked 2 inline comments as done.

Adjust to code review:

  • move Stepper.Items directly to Stepper.Container
  • replacing empty string with stringForUser function
web/modals/threads/create/steps/subchannel-members.css
7 ↗(On Diff #16927)

I put a reply in the previous update, but it seems that it disappeared.

Using position: sticky here allow us to keep searchbar always on the top of the modal. Other solutions could require some changes in structure of the component. I tried to avoid nesting a scrollable component in another scrollable component, thus using sticky here seems to be the simplest solution.

Here is a sample video that show the benefits of using it here:

So, I guess using sticky here is benefitable.

tomek added 1 blocking reviewer(s): atul.
tomek added inline comments.
web/modals/threads/create/compose-subchannel-modal.react.js
174 ↗(On Diff #17202)

We don't need keys anymore

web/modals/threads/create/steps/subchannel-members.css
7 ↗(On Diff #16927)

sticky provides sticky behavior (so the component could be at the top or lower, but never above the viewport). In this case we want it to always be at the top, so this position isn't necessary and there are other ways of achieving the desired result. We should definitely replace it, but given the short time period, you can create a task to address this.

Remove key prop from Stepper.Item

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

Accepting to unblock, but would be good to expand further on what needs to be implemented at the site of the TODO before landing.

web/modals/threads/create/compose-subchannel-modal.react.js
131 ↗(On Diff #17244)

Can you explain a little more what needs to be implemented here?

We typically don't want to leave TODOs in the codebase, but if we are it would be good to expand a bit.

This revision is now accepted and ready to land.Sep 30 2022, 8:08 AM
web/modals/threads/create/compose-subchannel-modal.react.js
131 ↗(On Diff #17244)

This is addressed in the next diff in the stack D5211