Scrollable modal

[ysds]

This feature is inspired by MDC Dialog.

The scrollable modal allows scrolling the modal body. The standard model height fits to own content height, but the scrollable modal fits within the viewport height if the content is too long.

demo: https://deploy-preview-27769--twbs-bootstrap4.netlify.com/docs/4.2/components/modal/#scrolling-long-content

Cheers!

[Johann-S]

Can you add a unit test please ?
Otherwise LGTM 👍

[ysds]

I'm not very familiar with QUnit. I'd like to ask your support 🙏

[Johann-S]

@ysds you have to check when you a modal is shown, if this modal has .modal-dialog-scrollable class, the modal body scrollTop property should be equal to 0

[MartijnCuppens]

@ysds, you'll need to add a test to this file: https://github.com/twbs/bootstrap/blob/v4-dev/js/tests/unit/modal.js
The scenario Johann describes should be a good test. I think you'll find out how the tests work if you look at other tests, but if encounter problems with it, let us know.

[tmorehouse]

I wonder if the scrollable content should be default behaviour on vertically centered modals?

[ysds]

@Johann-S I think that test result will always be OK, because the initial scrollTop of a modal body is 0. After creating a modal that overflows the height of the viewport, then it is necessary to change the scroll potion. How can I define a modal that overflows the height of the viewport? What is the height of the viewport in the test?

(I have never written a Qunit test script)

[Johann-S]

@ysds it seems not see: https://coveralls.io/builds/20490418/source?filename=js/src/modal.js#L251
If it was the case this line would be covered.

You have to create a modal with a height greater than window.outerHeight to overflow the height of the viewport.
A QUnit test or a unit test in a DOM, is the same as writting JS, you have to create the HTML a be sure your logic works

[ysds]

Done. I had some misunderstanding. It seems there was no need to get the viewport height.

[MartijnCuppens]

The css LGTM

[Johann-S]

LGTM 👍

[XhmikosR]

Rebased.

[XhmikosR]

@mdo: Is there something else we need here or can we merge?

[ysds]

Please wait to merge! I found a better workaround for the strange scrollbar behavior on IE.

[ysds]

I changed the modal.css only and force-pushed. Sorry but please review again 🙏

Latest changes not reviewed yet

[mdo]

Excited for this! Thank you <3.