Page MenuHomePhabricator

[web] Introduce `CommunityCreationMembersModal`
ClosedPublic

Authored by atul on May 18 2023, 12:32 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 26, 2:38 PM
Unknown Object (File)
Tue, Nov 26, 12:27 PM
Unknown Object (File)
Thu, Nov 14, 11:26 AM
Unknown Object (File)
Thu, Nov 14, 11:26 AM
Unknown Object (File)
Thu, Nov 14, 11:26 AM
Unknown Object (File)
Thu, Nov 14, 11:23 AM
Unknown Object (File)
Thu, Nov 14, 11:00 AM
Unknown Object (File)
Mon, Nov 4, 1:44 PM
Subscribers

Details

Summary

This modal is displayed after the community has been created and allows users to optionally add members.

Test Plan

Looks as expected:

219620.png (1×1 px, 122 KB)

Works as expected:

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul published this revision for review.May 18 2023, 12:40 PM
atul added inline comments.
web/sidebar/community-creation/community-creation-members-modal.css
8–36 ↗(On Diff #26661)

This is copy/pasted, will pull out the keyserver label into separate component in subsequent diff

web/sidebar/community-creation/community-creation-members-modal.react.js
26–32 ↗(On Diff #26661)

This is copy/pasted, will pull out the keyserver label into separate component in subsequent diff

Generally looks good, but confused about some of the changes to existing modals...

Can you explain what the impact is there, ideally with some screenshots?

web/modals/threads/members/members-modal.css
2 ↗(On Diff #26661)

What impact does this change have on existing modals? I'm a little confused why it's in this diff

web/modals/threads/members/members-modal.react.js
134 ↗(On Diff #26661)

What impact does this change have on existing modals? I'm a little confused why it's in this diff

web/sidebar/community-creation/community-creation-members-modal.css
8–36 ↗(On Diff #26661)

Would've preferred to have seen that diff first since it would've avoided the copy-paste

web/sidebar/community-creation/community-creation-members-modal.react.js
13 ↗(On Diff #26661)

For flexibility

Generally looks good, but confused about some of the changes to existing modals...

Can you explain what the impact is there, ideally with some screenshots?

Basically there are three "modal sizes": small, large, and fit-content. Using small and large makes sense because we're setting the dimensions of the "container"/parent and having the elements arranged within via flexbox. fit-content is weird because it "flips things." The size of the container/parent (modal) is dependent on the contents.

On the surface that seems fine, but it means eg that because AddMembersModalContent has a hard-coded width, it isn't "flexible" and can't be used in modals of different size. For example if we try to consume AddMembersModalContent within CommunityCreationMembersModal, we get something like this:

1ccfa5.png (1×1 px, 120 KB)

By removing the hard-coded width, we get something like:

e940a8.png (1×1 px, 119 KB)

which is what we want.

The problem then is in MembersModal where AddMembersModalContent is also consumed. Because we had fit-content there, and we aren't hard-coding a width, the modal looks very skinny:

15a125.png (1×790 px, 95 KB)

so I "rounded up" to large (instead of moving the hardcoded width somewhere else), and got:

edd69e.png (1×1 px, 103 KB)

If we really want to preserve the previous width we could possibly introduce a medium size modal, but I think the sizing here is reasonable. This approach ("named" modal sizes) is imo way easier to work with/adjust than hard-coding widths in CSS.

web/sidebar/community-creation/community-creation-members-modal.react.js
13 ↗(On Diff #26661)

There's some cascading flow stuff that this change introduces that comes down to how modal.react.js types the onClose prop:

4ba40a.png (138×704 px, 54 KB)

It might make sense to just have a followup diff that changes this broadly?

atul added inline comments.
web/sidebar/community-creation/community-creation-members-modal.css
8–36 ↗(On Diff #26661)

Agree in hindsight that this should've been handled in a previous diff

This revision is now accepted and ready to land.May 19 2023, 10:47 AM
This revision was landed with ongoing or failed builds.May 19 2023, 11:40 AM
This revision was automatically updated to reflect the committed changes.