Issue 683: calculating height of iframe with borders and/or padding #764
Conversation
There was a problem hiding this comment.
H, thanks for this, it looks great, the one comment I have is that it would be better to use getComputedStyle when getting padding and border sizes, so that any unit of measure can be used in the CSS
https://developer.mozilla.org/en-US/docs/Web/API/Window/getComputedStyle
|
It already is using getComputedStyle (line 159), but I think I have put an unnecessary check for 'px' string on the border widths. I guess that's why it looks, at first glance, like a regular style property. I'll take a look at that tomorrow. |
package.json
Outdated
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "iframe-resizer", | |||
| "version": "4.2.2", | |||
There was a problem hiding this comment.
Can you please leave this as 4.2.2. My deploy script will up the version number automagically
CHANGELOG.md
Outdated
| @@ -1,5 +1,6 @@ | |||
| # Version History | |||
|
|
|||
| - v4.2.3 [#683](https://github.com/davidjbradshaw/iframe-resizer/issues/683) Include border top/bottom, plus padding top/bottom, when calculating heights on iframe with `box-sizing: border-box;` | |||
There was a problem hiding this comment.
Don’t forget to add a credit link back to your GitHub account :)
There was a problem hiding this comment.
Thanks - wasn't sure how links worked at first, but I get it now
|
I've removed the check for 'px' on border widths (as it will always be 'px') and realised I also needed to strip off the 'px' from padding values before parsing them (fresh set of eyes). Also added a test using 'rem' units to check it's working as expected with units other than 'px'. |
src/iframeResizer.js
Outdated
| return top + bot | ||
| } | ||
|
|
||
| function getBorderEnds(compStyle) { | ||
| if (compStyle.boxSizing !== 'border-box') { | ||
| return 0; | ||
| } | ||
| var top = compStyle.borderTopWidth && compStyle.borderTopWidth.indexOf('px') !== -1 ? parseInt(compStyle.borderTopWidth.replace('px', ''), 10) : 0 | ||
| var bot = compStyle.borderBottomWidth && compStyle.borderBottomWidth.indexOf('px') !== -1 ? parseInt(compStyle.borderBottomWidth.replace('px', ''), 10) : 0 | ||
| var top = compStyle.borderTopWidth ? parseInt(compStyle.borderTopWidth.replace('px', ''), 10) : 0 |
There was a problem hiding this comment.
I don’t think you need replace here, as parseInt('10px') === 10
There was a problem hiding this comment.
Oh, right. I didn't realise the 'px' got stripped out automatically. I've pushed an update.
I want to leave the radix (10) in there though, as mozilla docs recommend "always specify a radix when using parseInt" because of some browsers defaulting to other values (it doesn't specify which browsers though unfortunately) - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt
There was a problem hiding this comment.
Hi David. Was there some reason this hasn't been approved yet? Happy to discuss.
|
Released in V4.2.3, sorry for the delay |
|
Cool, thanks. |
Addresses #683
Includes border top/bottom, plus padding top/bottom, when calculating heights on iframe with
box-sizing: border-box.