Skip to content
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

Merged
merged 1 commit into from
Mar 13, 2017

Conversation

ValentinH
Copy link
Contributor

As discussed in #9, I set the callOnAdd : false option on the detector to prevent onResize() 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...

@okonet
Copy link
Owner

okonet commented Mar 10, 2017

What GHI should this fix?

@ValentinH
Copy link
Contributor Author

What is GHI?

@okonet
Copy link
Owner

okonet commented Mar 13, 2017

@ValentinH GitHub Issue :)

@ValentinH
Copy link
Contributor Author

There is no issue open directly referring to this PR. However, the false-positive test in #9 was due to onResize() being called twice on load.

This is more about optimization because I don't see the point of having onResize() called twice when the component mounts.

@okonet okonet merged commit b73c96c into okonet:master Mar 13, 2017
@okonet
Copy link
Owner

okonet commented Mar 13, 2017

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) => {
Copy link
Owner

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?

Copy link
Contributor Author

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.

Copy link
Owner

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.

Copy link
Contributor Author

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! 😄

Copy link
Owner

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants