Center vertically in addition to horizontally

[voxpelli]

Extending on #12 by introducing the same method to vertically center the modal. The vertical center used is one based on the golden ratio to make it look more balanced than an exact 50% vertical center.

I understand that this is a breaking change, but perhaps it can be of interest still. The one thing in my initial impression that struck me as a bit odd was that the modal wasn't centered vertically.

For now I will just use the modalStyles option to add this centering myself to my own modals.

[GitCop]

There were the following issues with your Pull Request

• Commit: f449da6e571b5b150c581f123b6dd8bafd36b3c6
  • Your commit message body contains a line that is longer than 72 characters

---

This message was auto-generated by https://gitcop.com

[Nycto]

Thanks for the contribution! I'm actually working on a 3.0.0 right now to add accessibility support; it already has a number of breaking changes, so I'll evaluate this as an addition there

[Nycto]

I realized that this will fail when a modal is really tall. The top of the modal will be pushed off the top of the viewport. Of course, the existing behavior isn't great for this, either. The bottom of the modal just hangs off the bottom of the screen and can't be scrolled to.

[voxpelli]

To solve that one could add:

'max-height': '76.3932%',
overflow: 'auto',

That would ensure that it always fits within the screen and add a scrollbar whenever it doesn't

[voxpelli]

Added that solution + rebased on the 3.0.0 branch

[Nycto]

Still no dice, unfortunately. For really long modals, the top will still spill off the top of the screen. I added a test, here:

https://github.com/Nycto/PicoModal/commit/49bfd34c29de2c738a2d657b3ec01d20faaf3831

Which you can see running here:

https://saucelabs.com/tests/0bd15aa9b5d54187a461baa1fd9c20d4

(I also had to add a fix to the existing implementation to get it to pass, so your commit may have a merge conflict now. Apologies for the moving target)

[voxpelli]

That is a very odd failure. The modal spilling off the top of the screen when the content is too long is just weird. As top: is set to 50px and there's no vertical transforms or anything in the code revision you linked to, any overflow should be at the bottom of the screen, not the top.

Also – looking at the height of the modal it seems to be able to limit it, so I'm not sure if it's the height-limit that's the issue here or if it's some other positional issue?

(Also not sure how I can run these tests myself to try to reproduce :stuck_out_tongue:)

[Nycto]

(Also not sure how I can run these tests myself to try to reproduce :stuck_out_tongue:) Hide all checks

Haven't documented this yet; sorry.

To run the tests locally, run this:

npm install;
grunt dev;

That will spin up a local server running the tests. Go to:

http://127.0.0.1:8080

[Nycto]

That is a very odd failure. The modal spilling off the top of the screen when the content is too long is just weird. As top: is set to 50px and there's no vertical transforms or anything in the code revision you linked to, any overflow should be at the bottom of the screen, not the top.

Agreed. Took me a few minutes of fiddling with the CSS to figure out what was going on.

If you turn off the top: 38.1966% style, you can see what is happening. The translate(-50%, -50%) means that positioning is relative to the exact middle of the modal. So the center-line of the modal of at 38% of the page height. When modal.height * 50% > viewport.height * 38%, the modal shoots off the top of the page. Modal height tops out at 75% of the viewport height, but the additional padding is throwing it off.

/convoluted

I think adding box-sizing: border-box might help.

[voxpelli]

+1 on adding box-sizing: border-box, then if someone changes the padding or border the centering height limit and positioning will still work

[voxpelli]

Updated patch to include box-sizing: border-box

[voxpelli]

And I know realize that this PR points to the master-branch still – should I open another one for the 3.0.0 branch?

[Nycto]

I don't love how close the top of the modal gets to the top of the viewport when the modal gets tall-ish. What about something like this:

https://github.com/Nycto/PicoModal/commit/ad1ea88fd3c96bc46f64df87a2d5b604278da991

I amended your commit so it aligns the 38% vertical mark of the viewport with the 38% vertical mark on the modal

[Nycto]

To answer your question: No need to open another PR. I can just cherry pick your changes

[voxpelli]

After thinking some more about this, I'm not sure whether this should be the default or not. It's great for small position: fixed modals which was my first use of this library, but now in my second use of it I instead had a very large long modal and thus opted for a non-vertically centered position: absolute modal that had a top-offset of something like (window.pageXOffset + 50) + 'px'.

I would probably say:

1. When position: fixed is set, then this vertical centering would be preferred
2. For cases where 1 doesn't work, then instead do a position: absolute

Would it be a good idea to support both these two cases built into the module? Or may it be better to just provide a bare minimum and leave most of it up to the user?

[Nycto]

I merged in your change, but tweaked it so it doesn't get as snug to the top. You can see the changes I made here:

https://github.com/Nycto/PicoModal/commit/b40b8a14b7119e0d85ee5baab5b718e1e789c85b

This is what the result looks like for really tall modals:

https://assets.saucelabs.com/jobs/818dab1d540f45559393b01dfc4394d1/0011screenshot.png