feat: Add specific padding to boundaries (#518) #615
Conversation
| boundaries.left += padding.left || padding; | ||
| boundaries.top += padding.top || padding; | ||
| boundaries.right -= padding.right || padding; | ||
| boundaries.bottom -= padding.bottom || padding; |
There was a problem hiding this comment.
We should probably fallback to 0 in case one of the sides is not defined (eg: {top: 10, left: 20 }
There was a problem hiding this comment.
This should be updated now with the last squash+push
|
May you add few tests to cover the new cases please? |
|
@FezVrasta After I get these tests done, how long would it be until this is in a version? I have a week until code freeze, so need to either fork this and go off that, or need to have these changes in. |
|
As soon this is merged I'll release the new version with this feature in. |
|
OK I am working on a few other higher priority issues, I'll get some tests in as soon as I can. |
|
@FezVrasta I created some tests around the method, also tweaked your changes to support nulls. Ahh IE tests are failing and I am on a Mac. Can you help with that please? |
|
You can get an Internet Explorer VM for free from here: I don't have time to look into it by myself unfortunately. |
BrianBu01
force-pushed
the
#518
branch
11 times, most recently
from
ccc12df
to
efc5183
Compare
|
@FezVrasta Tests are added and passing. Some Tests are not passing for IE10 and IE11, but those tests were a backfill for missing tests for existing functionality. |
|
@FezVrasta This is ready for your review. |
|
I don't get why the tests are not passing on IE...this isn't any browser specific functionality |
|
@FezVrasta I'm not sure either, but those tests were just backfill only related due to proximity. Can we still get these changes in now, and one of us can take a closer look when we have more time? I have to pull this into my current project today, and I'll have to go off my clone if it isn't in yours. |
|
I'd rather wait until we have a solid pr sorry |
|
@FezVrasta Since those test don't have to with the change, would just removing them, make the pr "solid" for you? |
|
No sorry. If they work on master they must work here as well. I'll be able to better look at this PR this evening anyway |
|
I tried debugging the tests on another PR, as I am not sure how to get the IE tests to run locally even under a VM. Pretty much I think IE is doing something funny in getBoundingClientRect. |
| computedStyle[`margin${axis === 'Height' ? 'Top' : 'Left'}`] + | ||
| computedStyle[`margin${axis === 'Height' ? 'Bottom' : 'Right'}`] | ||
| : 0 | ||
| ? (parseInt(html[`offset${axis}`]) + |
There was a problem hiding this comment.
These are strings in IE10 such as 0px, so you are doing string concatenation here instead of addition. You end up with 1070px0px0px. Then maxing with that param causes NAN to be returned.
|
@FezVrasta Well there you go. |
|
@FezVrasta Could you please publish a new version for this? Thanks! |
There were no tests for this class already, but all existing tests still pass.