Bugfix maxHeight after $.colorbox.resize() #635
Conversation
Earlier, if I set .colorbox{maxHeight:'95%'...} then colorbox frame gets vertical scrollbar (if content is heighter than frame) but if then I do $.colorbox.resize() my colorbox frame becomes heighter than wrapper page and my maxHeight option is ignored, so this is bugfix for it.
| @@ -733,6 +733,7 @@ | |||
| scrolltop = $loaded.scrollTop(); | |||
| $loaded.css({height: "auto"}); | |||
| settings.h = $loaded.height(); | |||
| if (settings.h > settings.mh) settings.h=settings.mh; | |||
There was a problem hiding this comment.
Wouldn't following code be more explicit:
settings.h = Math.min(settings.h, settings.mh);There was a problem hiding this comment.
Great, please make necessary adjustments to your PR code then. Also don't forget to squash the commits.
There was a problem hiding this comment.
Maybe then settings.h = Math.min($loaded.height(), settings.mh); ?
2014-08-13 11:55 GMT+03:00 Alexander Obuhovich notifications@github.com:
In jquery.colorbox.js:
@@ -733,6 +733,7 @@
scrolltop = $loaded.scrollTop();
$loaded.css({height: "auto"});
settings.h = $loaded.height();
if (settings.h > settings.mh) settings.h=settings.mh;Great, please make necessary adjustments to your PR code then. Also don't
forget to squash the commits.—
Reply to this email directly or view it on GitHub
https://github.com/jackmoore/colorbox/pull/635/files#r16164162.
There was a problem hiding this comment.
It's your PR. I've just modified your code to make it easier to read. Honestly I don't even understand what you've changed in code and how exactly it helps in solving the problem.
There was a problem hiding this comment.
Here is few jsfiddles:
- With original script: http://jsfiddle.net/srhn524o/
- With fixed script: http://jsfiddle.net/1gdgh4kj/
Press Open and the Resize button
2014-08-13 12:00 GMT+03:00 Alexander Obuhovich notifications@github.com:
In jquery.colorbox.js:
@@ -733,6 +733,7 @@
scrolltop = $loaded.scrollTop();
$loaded.css({height: "auto"});
settings.h = $loaded.height();
if (settings.h > settings.mh) settings.h=settings.mh;It's your PR. I've just modified your code to make it easier to read.
Honestly I don't even understand what you've changed in code and how
exactly it helps in solving the problem.—
Reply to this email directly or view it on GitHub
https://github.com/jackmoore/colorbox/pull/635/files#r16164305.
There was a problem hiding this comment.
One, that you indicated as fixed do nothing. I click resize and nothing happens. The original script resized colorbox beyond possible screen space.
There was a problem hiding this comment.
It resizes it to overflow page if contents is too large even if I set
maxHeight: 95%.
13.08.2014 12:35 пользователь "Alexander Obuhovich" <
notifications@github.com> написал:
In jquery.colorbox.js:
@@ -733,6 +733,7 @@
scrolltop = $loaded.scrollTop();
$loaded.css({height: "auto"});
settings.h = $loaded.height();
if (settings.h > settings.mh) settings.h=settings.mh;One, that you indicated as fixed do nothing. I click resize and nothing
happens. The original script resized colorbox beyond possible screen space.—
Reply to this email directly or view it on GitHub
https://github.com/jackmoore/colorbox/pull/635/files#r16165855.
There was a problem hiding this comment.
Will you merge this? How to implement correct resize after window resize in
case of maxHeight is configured?
2014-08-13 12:46 GMT+03:00 Alexander Obuhovich notifications@github.com:
In jquery.colorbox.js:
@@ -733,6 +733,7 @@
scrolltop = $loaded.scrollTop();
$loaded.css({height: "auto"});
settings.h = $loaded.height();
if (settings.h > settings.mh) settings.h=settings.mh;OK, now I get it.
—
Reply to this email directly or view it on GitHub
https://github.com/jackmoore/colorbox/pull/635/files#r16166268.
I don't have merge rights on this repo.
No idea. |
|
+1 I've encountered this problem before. Thanks @lufton. |
|
+1 This fix worked for me, thank you. |
|
Just tested this, works fine. Thanks @lufton. Needs merging, current code ignores maxHeight when resizing. Tested in a site that makes heavy use of colorbox (multiple "nested" colorboxes by changing its content, image galleries, product quick views, etc...) I'm not sure it should default to 100% max height though, because it could possibly affect current implementations. In my opinion it should maintain current behaviour when no maxHeight is set. |
Earlier, if I set .colorbox{maxHeight:'95%'...} then colorbox frame gets vertical scrollbar (if content is heighter than frame) but if then I do $.colorbox.resize() my colorbox frame becomes heighter than wrapper page and my maxHeight option is ignored, so this is bugfix for it.