LR-217 Modal content size updates (Slight API change)

[namick]

Changes

• Add a size={2} option to allow vertical size increase of Modal up to screen size minus margin
• Update the fullWidth option to allow horizontal size increase of Modal up to screen size minus margin

Purpose

PHC and other products have fairly complicated UI's that require more flexibility than two Modal sizes. The existing solution is to use the full-screen option. However, there are a couple reasons the full-screen option is not adequate for some use cases.

1. Overkill. The largest non-full-screen option is still fairly small. When a UI requires Modal only slightly larger than this, it must use the full-screen option. The result can be a lot of extra whitespace and a suboptimal user experience.
2. Loss of context. The full-screen Modal option isn't great for sub-tasks because it is more like navigating to a new page. While editing the many options of a Survey question for example, there is no visual reminder of the main task because the whole screen is hidden. This is compounded when you have a full-screen Modal opening another full-screen Modal.

Currently the width and height of the Modal are controlled by the values of fullWidth and size.

• fullWidth={false} causes the model width to be set to a hard 480px
• fullWidth={true} causes the model width to be set to a hard 600px
• size={0} causes the model height to be able to expand with the content up to 256px
• size={1} causes the model height to be able to expand with the content up to 480px

Content larger than the above values becomes scrollable within the Modal. This is sometimes okay for vertical scrolling but horizontal scrolling is almost always suboptimal.

BEFORE

modal-updates-before.mp4

Non-breaking change

The first change this PR proposes is an additional size value of 2. When set to size={2} the height is allowed to expand with the content up to the size of the screen minus a significant margin of 3rem (48px) on the top and bottom. This won't affect any existing apps.

Breaking change

The second change is with fullWidth={true}. Previously this produced a hard width of 600px. Now the width starts at a minimum of 600px, but allows the content to expand the Modal up to the width of the screen minus a margin of 3rem (48px) on the left and right.

Technically a breaking change in the API, but in practice, I'm unaware of any use of the Modal that relies on horizontal scrolling of it's content. As mentioned, a slightly wider Modal is almost always better than horizontal scrolling.

Near full-screen

If required by the design, the PR allows for the creation of near full screen Modals that allow for a lot of UI real estate, yet retain a significant border of 48px, enough to provide a visual reminder of the main task a user is involved with.

AFTER

modal-updates-after.mp4

Consistency

We don't want a Modal that only expands to whatever size it's content happens to be. This could produce an app with an handful of randomly sized Modals over time. This PR attempts to keep existing defaults and several good presized options, while at the same time providing the ability create a new size.

When a specific design calls for a bunch of Modals opening at 400x750 to accommodate the UI, this makes that possible. It is easy to limit the size of the content itself, allowing the Modal to constantly enclose itself around that content.

PHC BEFORE:

modal-update-phc-before.mp4

PHC AFTER

modal-update-phc-after.mp4

[namick]

I will update the documentation / README in the story if we think this API change is good.

[dexterca]

After some discussion with @ynotdraw and @namick, we're going to go ahead and move forward with replacing size and fullWidth with height and width, where height and width will have four different size options (0-3) for each prop.

[markdlv]

Sounds like the discussion went well? Please don't merge with my approval alone, just wanting to show support as this LGTM!

[sternetj]

Where does 480 come from? Should it be based off of one of the breakpoints? If so, should we be using a breakpoint function here so it gets the sizes from the theme?

[namick]

Indeed. This PR was my attempt to make as few changes as possible to give us what we needed. 480 was previously hard-coded. We got together yesterday and decided to do a more extensive, non-backward compatible upgrade on this component with new prop names as well as themed breakpoints.

[namick]

Sounds like the discussion went well? Please don't merge with my approval alone, just wanting to show support as this LGTM!

Yes, I was being a bit conservative in trying not to make breaking changes. We have come a long way since this was originally created and want to move forward with prop changes that make more sense.

[namick]

I'm going to close this for now since I don't have time to work on it. Will reopen when I get back to it.