Add Full-Screen size to Modal and Responsive variations for breakpoints

[GeoSot]

Add Full-Screen size to Modal and Responsive variations for breakpoints
Works from breakpoint and down

( @MartijnCuppens , please ignore my previous pull request)

Preview: https://deploy-preview-28683--twbs-bootstrap.netlify.com/docs/4.3/components/modal/#fullscreen-modal
Previous PR: #28682

[MartijnCuppens]

Yu @GeoSot,

Nice to see you getting involved in Bootstrap! I've readded the netlify preview in your (you can get the link in the PR tests). Try not to reopen too many PR's for the same feature, we can miss some essential information sometimes.

I noticed there's a spacing at the right side of the screen, could you fix that?

[GeoSot]

@MartijnCuppens you are right about the two PR's. (I 've messed up the commits and wanted a clear solution)

As for the padding on the right, I saw that it is done from js.
As a solution I can change max-width to min-width, in order to have a nice optical result, but is it acceptable?

[MartijnCuppens]

Hmmm, I can't think of another better solution right now. Maybe @ysds knows a better way of handling this?

[ysds]

I’ll take a look later :)

[ysds]

As far as looked through that, I felt three things:

• The class name is a bit confusing. e.g. I misunderstood the .modal-md-full makes the modal to be full screen when the screen is wide.

• There is a display problem when the full screen modal has a lot of content.

• The modal's right padding is for centering the modal to the page content not the window (Modal layer is centered based on window, not based on content #14892). But the full screen modal will not require the padding. To remove the padding, should change the JS because it is added by JS.

[GeoSot]

@MartijnCuppens I have made some changes and fixes according to the previous suggestions.

As for the className, i am open to suggestions

[MartijnCuppens]

As for the className, i am open to suggestions

fullscreen would make more sense I guess.

There's something else. We use a mobile-first approach in Bootstrap, which uses min-width media queries instead of max-width. We also need extra documentation and examples to show the responsive effect.

[GeoSot]

There's something else. We use a mobile-first approach in Bootstrap, which uses min-width media queries instead of max-width. We also need extra documentation and examples to show the responsive effect.

I have changed the class name.

As for the mobile first approach thing, the purpose is to have a Full Screen modal on mobile , till the chosen breakpoint. So I am not sure if it needs any change

[XhmikosR]

@GeoSot something seems broken. Also please squash your patches into one after rebasing against upstream.

[GeoSot]

@XhmikosR can I do something more in order to help this pull-request to proceed?

[mdo]

Trying this out myself, I don't think the fullscreen modal should "drop in" like the other modals—gives it a weird looking animation. Thoughts on revisiting that part?

[GeoSot]

Maybe if we omit the 'fade' class. Or try another transition???

[MartijnCuppens]

I would keep the fade effect. Maybe we split the fade effect and the slide-in effect into 2 classes? Better cover this in another PR then because all modal classes.

[GeoSot]

So, what you propose me to do?

[MartijnCuppens]

So, what you propose me to do?

Interesting question. I know @ysds is working on splitting the transition classes from the components in #29081.

@mdo, maybe we should postpone the drop in change until #29081 is finished?

[mdo]

Sounds fine by me!

[ysds]

Since there are still some issues, I will add the commit directly.

Adding overflow-y: auto to the global .modal-body will increase encountering #28513 (See demo). Maybe should handle with JavaScript, but for workaround here, moving it to the each .modal-body.

[ysds]

I added changes:

• Fix header and footer shrink issue
• Move overflow-y: auto from the global .modal-body to each fullscreens
• Use height: 100% instead of 100vh because 100vh doesn't care the browser UI (e.g. address bar)
• Fix wrong breakpoint postfixs and remove modalFullscreen() mixin
• Improve documentation

[ysds]

LGTM, but need another approval from @twbs/css-review

[MartijnCuppens]

I've added border radii resets to the footer and header, just in case a background color is added. Thanks all for contributing to feature! ❤️

[MartijnCuppens]

@XhmikosR, how are we gonna merge this? Squash it into one commit?

[XhmikosR]

This should probably me manually rebased and squash the patches per author.

[XhmikosR]

Unless you guys don't mind squashing everything into one when merging (with a clean commit message, though)

[MartijnCuppens]

I personally don't care. We could maybe add Co-authors in the commit message:

Co-authored-by: XhmikosR <xhmikosr@gmail.com>
Co-authored-by: Shohei Yoshida <fellows3@gmail.com>
Co-authored-by: Martijn Cuppens <martijn.cuppens@gmail.com>

[XhmikosR]

@ysds can you have another look after the rebase?

@GeoSot please make sure you don't overwrite my rebase 🙂

[ysds]

All looks good to me 👍