Page MenuHomePhabricator

[web] Redesign `Modal` to match Figma
ClosedPublic

Authored by jacek on Apr 8 2022, 5:54 AM.
Tags
None
Referenced Files
F3174740: D3669.id11226.diff
Thu, Nov 7, 5:52 PM
Unknown Object (File)
Wed, Oct 23, 4:28 PM
Unknown Object (File)
Wed, Oct 23, 4:28 PM
Unknown Object (File)
Wed, Oct 23, 4:28 PM
Unknown Object (File)
Wed, Oct 23, 4:28 PM
Unknown Object (File)
Wed, Oct 23, 4:28 PM
Unknown Object (File)
Wed, Oct 23, 4:28 PM
Unknown Object (File)
Tue, Oct 22, 2:07 AM

Details

Summary

Redesigned modal to match existing designs. Refactored and simplified CSS classes and removed redundant container in component.
Removed fixedHeight property, as it was used in one place and had no effect on existing implementation.
Existing modal sizes (small, large) should work as before. Introduced fit-content size option, to allow adjust modal size by its content.

Screenshot_Google Chrome_2022-04-08_150355.png (895×1 px, 114 KB)

Test Plan

Opened currently existing modals. It's size should remain unchanges, but fonts and design should now match Figma.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

The modal in the provided screenshot looks way better!

web/modals/modal.css
8 ↗(On Diff #11226)

I think we could add colors like this (w/ alpha channel) to the theme.css file... but I think it's fine either way

25 ↗(On Diff #11226)

Just a thought (and outside the scope of this diff), but I wonder if naming these modalContainerNarrow and modalContainerWide would make it clear that the difference is just along the X-axis?

27 ↗(On Diff #11226)

Is specifying min-height: 0 necessary? I'm assuming you found that you needed to specify this to get things to look right, just making sure it wasn't included extraneously?

This revision is now accepted and ready to land.Apr 8 2022, 11:17 AM
web/modals/modal.css
27 ↗(On Diff #11226)

Sorry, I missed this comment. Checked this, and it looks like it's not necessary and must have been added by accident.

removed redundant min-height

This revision was automatically updated to reflect the committed changes.