I'm just getting started with this module, I have to say, it's great! I works well and I haven't had any barriers in achieving what I want through the API. Very nice job.

So far the only minor nuisance is that the height calculation seems to be not-quite-suitable for my needs, since right now it's percentage based. A height of "98%" often ends up not working very well, since a top margin of 10 pixels seems to be hard-coded (though I'm not sure where). So a height of 98% often can lead to the bottom of the popup not being visible or just barely in the frame. Doing a little math:

Window height: 500 pixels
98%: 490 pixels
10 pixels: 10 pixels ;-)

This means that if the window is any smaller than 500 pixel in height, the bottom of the dialog starts getting cut off.

Window height: 400 pixels
98%: 392 pixels
392 height + 10 pixel top margin = 402 height for modal, cutting off the bottom slightly.

So what I'd like to propose is not to use percentage-based heights and instead just enforce a certain margin around the outside. This patch makes a 20 pixel vertical margin (excluding the forced 10 pixel top margin, which will now be 30 pixels) and a 30 pixel horizontal margin.

CommentFileSizeAuthor
#1 modalframe_height.patch984 bytesquicksketch
modalframe_height.patch1004 bytesquicksketch

Comments

quicksketch’s picture

quicksketch
he/him
commented
StatusFileSize
new modalframe_height.patch984 bytes

I suppose we don't need that parseInt() in there if we're now only working with integers.

markus_petrux’s picture

markus_petrux commented
Status: Needs review » Needs work

hmm... using 98% was suggested by Gábor here #491224: Port modalframe API to Drupal 7, related to d7ux (IIRC, I initially coded this to use 85%). Honestly, I liked that (bigger than 85%), specially when using modal frames within other modal frames.

I'm fine with your proposed change, however, how about using window dimension - 30px?

-  var minWidth = 300, maxWidth = parseInt($window.width() * .98);
+  var minWidth = 300, maxWidth = $window.width() - 30;

-  var minHeight = 100, maxHeight = parseInt($window.height() * .98);
+  var minHeight = 100, maxHeight = $window.height() - 30;

Does 30 work for you? Maybe 40?

quicksketch’s picture

quicksketch
he/him
commented

That works fine for me, anything will work fine as long as it's an exact pixel amount instead of a percentage. :-)

markus_petrux’s picture

markus_petrux commented
Status: Needs work » Fixed

Ok, thanks. Good one. I also like this better than using percents. Committed as in #2 (patch).

For the record: This is just the default dimension of the dialog. The caller can specify explicit dimensions using the width and height arguments in Drupal.modalFrame.open(options); see project page and README for details.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.