[web] cleanup community creation members modal
ClosedPublic
Actions

Authored by ginsu on Feb 11 2024, 10:15 PM.

Details

Reviewers

atul
inka

Commits

rCOMMf231eeef1f7e: [web] cleanup community creation members modal

Summary

This diff cleans up the community creation modal to follow the conventions of the new modals api and additionally use the new add members search experience.

Linear task: https://linear.app/comm/issue/ENG-6308/avatar-appears-above-search-experience-in-channel-creation-modal

Depends on D11023

Test Plan

Please see the demo video below

Also confirmed that with the new search experience the avatar does not appears above search bar

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ginsu created this revision.Feb 11 2024, 10:15 PM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptFeb 11 2024, 10:15 PM

Harbormaster completed remote builds in B26733: Diff 36965.Feb 11 2024, 10:31 PM

ginsu requested review of this revision.Feb 11 2024, 10:31 PM

ginsu edited the summary of this revision. (Show Details)Feb 11 2024, 10:37 PM

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

ginsu added reviewers: atul, inka.

ginsu added a parent revision: D11023: [web] cleanup add members modal.

ginsu attached a referenced file: F1144075: Screen Recording 2024-02-12 at 1.36.04 AM.mov. (Show Details)

ginsu attached a referenced file: F1144059: Screen Recording 2024-02-12 at 1.18.31 AM.mov. (Show Details)

ginsu added a child revision: D11025: [web] cleanup UnsavedChangesModal.Feb 11 2024, 10:43 PM

Looks good, some notes inline. Probably makes sense to create tasks and link before landing instead of addressing right now.

web/sidebar/community-creation/community-creation-members-modal.css
5 ↗	(On Diff #36965)	Since we're using relative units here, would be good to see something in Test Plan about testing at different window heights. Don't need screenshots and screen recording, but just a note that you made sure things look okay. Main concern is if some dimensions are relative and some are fixed that everything continues to look decent.
web/sidebar/community-creation/community-creation-members-modal.react.js
38–45 ↗	(On Diff #36965)	Do we not want to make sure `callChangeThreadSettings` succeeded before closing the modal? In other modals we show a spinner and then display an error if the request fails so the user can try again. Can look to eg `ThreadSettingsModal` as an example.
73 ↗	(On Diff #36965)	We should definitely definitely be memoizing this
83 ↗	(On Diff #36965)	Feel like we could probably avoid this `div`?
96–101 ↗	(On Diff #36965)	Let's memoize

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

ginsu added inline comments.Feb 15 2024, 2:05 AM

web/sidebar/community-creation/community-creation-members-modal.css
5 ↗	(On Diff #36965)	https://linear.app/comm/issue/ENG-6813/height-prop
web/sidebar/community-creation/community-creation-members-modal.react.js
38–45 ↗	(On Diff #36965)	This code is pretty much the same logic as in `AddMembersModal` (probs should have factored this out initially too rather than duplicate it) https://github.com/CommE2E/comm/blob/master/web/modals/threads/members/add-members-modal.react.js#L80-L95 Here is a liner task to track improving this experience: https://linear.app/comm/issue/ENG-6814/improve-add-members-modal-ux
83 ↗	(On Diff #36965)	We need these divs to prevent this: