Modal: add verticalScrollMode property to limit overflow to children

[timbotnik]

In some cases we want to be able to see the action buttons even if the child content overflows - this gives us a way to use a flex column layout and move the overflow-y to the div that wraps children.

[cheapsteak]

Is there a way to solve this by adding a less specific prop? (maybe expose classNames on the div that wraps the "actions"?)

Aside from the concern for option-bloat, verticalScrollMode="children" is a sequence of words that I might have trouble making a reasonable guess at what it does if I saw them at a usage site

It's also adding a flag argument / code smell that imo indicates the component might not be optimally factored; For comparison, the Modal components of material-ui, fluent-ui, semantic-ui, and chakra-ui do not make assumptions about the contents of a Modal the way we do (imo this is presently an overly-specific lego piece, although out of this PR's scope to solve)

I'm not sure how to balance "wanting a great api for a shared public ui lib" and "not wanting to block sprint work in the main project the lib is used for". Going to punt that question to other reviewers 🙈

[timbotnik]

Thx for the insightful review @cheapsteak. I agree with you on most of these points.

Is there a way to solve this by adding a less specific prop? (maybe expose classNames on the div that wraps the "actions"?)

I would have liked this as well, but it seemed like I needed to do these 3 things:

1. turn off overflow-y: auto on the parent div so that we'd be able to handle the overflow in descendants
2. make the layout display: flex; flex-direction: column so we could stretch the children area to fill the modal
3. set overflow-y: auto on the div that wraps children.

I think there is a slight alternative here where we could pass in classNames to get #1 & #2 and expose a childWrapperClassnames property for #3. It did feel pretty weird to me that we'd be passing in a flex layout which would depend on the nesting of things inside the modal to stay the same so I didn't pursue that option. It felt like this component had already made a call to be more of a layout owner, and this could certainly lead to options creep if we keep going this way.

I think ultimately what we might want is a way to separate a Modal (which handles the basic treatment of popping up a card) and the internal Modal layout as different things, allowing us to have different modal layouts or potentially fully customize the layout at usage site.

Aside from the concern for option-bloat, verticalScrollMode="children" is a sequence of words that I might have trouble making a reasonable guess at what it does if I saw them at a usage site

I toyed around with different names here like overflowYAt: 'children' | 'modal', but yeah not really the main problem here.

I'm not sure how to balance "wanting a great api for a shared public ui lib" and "not wanting to block sprint work in the main project the lib is used for". Going to punt that question to other reviewers 🙈

Based on time we need to ship something now and revisit it.

[cheapsteak]

Appreciate your patience in walking through the options considered 🙏

It felt like this component had already made a call to be more of a layout owner

First time hearing the term "layout owner", that's exactly it ⚡
The outward-facing option bloat is one part, the additional internal branching is another, both are a bit of a bummer

You're right though that this component has already decided to own layout and there doesn't seem to be a good way to invert control to usage sites without also requiring the user to read this component's internals

I think ultimately what we might want is a way to separate a Modal (which handles the basic treatment of popping up a card) and the internal Modal layout as different things, allowing us to have different modal layouts or potentially fully customize the layout at usage site.

Agreed. IMO Chakra/Semantic handle this wonderfully -- Modal is just the popup treatment, anything can be passed in as children, but they also provide ModalHeader/Modal.Header, ModalBody/Modal.Content etc helper components with preset styles.

Based on time we need to ship something now and revisit it.

👍

[timbotnik]

Merging but let's get this refactor on the agenda at some point.

[apollo-bot2]

🚀 PR was released in v7.6.0 🚀