Page MenuHomePhabricator

[web] cleanup change member role modal
ClosedPublic

Authored by ginsu on Feb 8 2024, 9:19 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 15, 4:17 PM
Unknown Object (File)
Thu, Oct 31, 8:38 PM
Unknown Object (File)
Thu, Oct 31, 6:55 AM
Unknown Object (File)
Thu, Oct 31, 6:55 AM
Unknown Object (File)
Thu, Oct 31, 6:55 AM
Unknown Object (File)
Thu, Oct 31, 6:55 AM
Unknown Object (File)
Thu, Oct 31, 6:55 AM
Unknown Object (File)
Thu, Oct 31, 6:55 AM
Subscribers

Details

Summary

This diff cleans up the one off styles in the change member role modal and replaces it with styles of the redesigned modal. Additionally we fixed the issue where the height of the modal changes when the dropdown open/closes. To fix this, I made the height of the change member role modal match the height of the window. I did this so that the height of the modal was consistent with other full screen height modals like the pinned messages modal + message search modal for example. However, if we think this is too much white space and it would be better to have the height wrap the maximum height when the dropdown is open, I can quickly make that change.

Linear task: https://linear.app/comm/issue/ENG-6315/change-role-modal-on-web-changes-height-and-position-when-dropdown-is

Depends on D11064

Test Plan

Please see the demo video below

Before:

after:

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Feb 8 2024, 9:24 AM
Harbormaster failed remote builds in B26655: Diff 36857!
ginsu requested review of this revision.Feb 8 2024, 9:45 AM

will make sure ci passes before landing

This revision is now accepted and ready to land.Feb 8 2024, 11:03 AM
ashoat requested changes to this revision.Feb 14 2024, 10:24 AM

However, if we think this is too much white space and it would be better to have the height wrap the maximum height when the dropdown is open, I can quickly make that change.

Yes this is too much whitespace...

This revision now requires changes to proceed.Feb 14 2024, 10:24 AM

test plan has been updated to reflect the new changes

This revision is now accepted and ready to land.Feb 14 2024, 12:50 PM
web/modals/threads/members/change-member-role-modal.react.js
149 ↗(On Diff #37105)

What's this <> for?

web/modals/threads/members/change-member-role-modal.react.js
149 ↗(On Diff #37105)

That was accidentally left there, I will clean it up

address comments + rebase before landing

This revision was landed with ongoing or failed builds.Feb 15 2024, 4:26 AM
This revision was automatically updated to reflect the committed changes.