[web] Remove `max-height` property from `modal.css: modalContainer[Small/Large]`
ClosedPublic
Actions

Authored by atul on May 2 2022, 12:42 PM.

Details

Reviewers

• jacek
tomek
• benschac
ashoat

Commits

rCOMM04371e4cee78: [web] Remove `max-height` property from `modal.css: modalContainer[Small/Large]`

Summary

Looks like this property was introduced in D3669. I understand fixing the width of the modals, but I think we can let the height expand to fit the content?

It doesn't seem right to arbitrarily cap the possible height of a Modal at this level of abstraction? I understand there might be the need to cap it for specific Modals?

For example, here's the ThreadSettingsModal before and after this diff:

Before:

After (and before D3669):

Test Plan

Go through every usage of Modal and make sure things looks as they did before.

(Haven't actually done this yet, but will do before landing)

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

atul created this revision.May 2 2022, 12:42 PM

Herald added subscribers: • abosh, • karol, • adrian. · View Herald TranscriptMay 2 2022, 12:42 PM

atul published this revision for review.May 2 2022, 12:42 PM

atul edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B8725: Diff 12152.May 2 2022, 12:48 PM

I'm not sure why @def-au1t added this, but I think it might be smart to wait for him to explain his thinking before landing

This revision is now accepted and ready to land.May 2 2022, 12:56 PM

In D3892#108848, @ashoat wrote:

I'm not sure why @def-au1t added this, but I think it might be smart to wait for him to explain his thinking before landing

Yeah makes sense, will add him as blocking

atul added 1 blocking reviewer(s): • jacek.May 2 2022, 1:00 PM

This revision now requires review to proceed.May 2 2022, 1:00 PM

As far as I remember, all modals before D3669 (the smaller and the larger ones) had the max-height set to 500px, so when redesigning I didn't want to change this size.
I'm open to removing this height limit, but please make sure it doesn't break existing modals.

This revision is now accepted and ready to land.May 4 2022, 7:46 AM

I'm open to removing this height limit, but please make sure it doesn't break existing modals.

Makes sense, I did a quick look through and everything seems to be fine.

Checked the following:

LogInModal
UserSettingsModal
InvalidUploadModal
SidebarListModal
ConcurrentModificationModal
HistoryModal
SearchModal
CantLeaveModal
ConfirmLeaveThreadModal
NewThreadModal
ThreadSettingsModal
PasswordChangeModal

It looks like SidebarListModal does extend the height indefinitely if a thread has a crazy amount of sidebars (and I'm guessing HistoryModal would as well if there were a lot of changes to an event), but I think the correct way to handle that would be to set a max-height on a div internal to SidebarListModal/HistoryModal when those get worked on.