-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set callOnAdd: false option of the detector #15
Conversation
What GHI should this fix? |
What is GHI? |
@ValentinH GitHub Issue :) |
There is no issue open directly referring to this PR. However, the false-positive test in #9 was due to This is more about optimization because I don't see the point of having |
Awesome! Thanks 🙏 |
@@ -49,15 +49,15 @@ describe('react-container-dimensions', () => { | |||
ContainerDimensions.prototype.onResize.restore() | |||
}) | |||
|
|||
it('calls onResize when parent has been resized', (done) => { | |||
xit('calls onResize when parent has been resized', (done) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see you skipped the test completely. I though this will fix it. Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, the test was passing for wrong reasons because it was a false positive. Now with my PR, the test fails.
I have tried to fix it but couldn't manage to send the resize event to the detector in JSDom. I think you mentioned that in another PR of mine, I'm not sure it's doable with JSDom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, makes sense. I'll remove the test for now then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it would be better to keep it so it's not forgotten! 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, decided to keep it :) Thanks!
As discussed in #9, I set the
callOnAdd : false
option on the detector to preventonResize()
to be called twice.However, I have excluded the resize test because it was failing (it was passing before, but it was a false positive) and I couldn't manage to trigger the resize from test...