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

Wrapper existence check doesn't work correctly #17

Closed
horaklukas opened this issue Mar 27, 2018 · 4 comments
Closed

Wrapper existence check doesn't work correctly #17

horaklukas opened this issue Mar 27, 2018 · 4 comments

Comments

@horaklukas
Copy link

horaklukas commented Mar 27, 2018

Using wrapperId to distinguish wrapper elements of more receivers doesn't work because of check for wrapper element existence in constructor.

Consider test below, it should pass, but it fails with "wrapper element with Id customId not found" error. Same happens when using it in real browser.

it('should not throw if wrapperId is given and element elements having that id exists', () => {
    expect(() => mount(
        <div id="customId">
          <Receiver
            wrapperId="customId"
            onDragEnter={emptyFn}
            onDragOver={emptyFn}
            onDragLeave={emptyFn}
            onFileDrop={emptyFn}
          />
        </div>
    )).not.toThrow();
});

It's probably caused by timing check, in component constructor is too early because whole elements tree haven't fully rendered yet.

@horaklukas horaklukas changed the title Wrapper existence check doesn't works correctly Wrapper existence check doesn't work correctly Mar 27, 2018
lionng429 added a commit that referenced this issue Mar 27, 2018
- as mentioned in #17, wrapper existence check does not work for wrapperId pointing to Receiver's parent wrapper. Moving the logic back to componentDidMount solves this issue.
@lionng429
Copy link
Owner

lionng429 commented Mar 27, 2018

@horaklukas Thanks so reporting the issue and this is a very good catch! I created a PR #18 to fix this issue.

FYI, the test case you mentioned above will always fail, not because of the placement of existence check, but the nature of mount() that it does not attach the component to the document, and this is the reference. I stuck a bit when adding the unit test, and I hope it would help in case you had the same problem.

Thanks once again for you time on checking the changes!

@horaklukas
Copy link
Author

Looks good, thanks for quick fix. Also big thanks for sharing the trick about mounting to the document, I did't know that before! 👍

@horaklukas
Copy link
Author

I will test it tomorrow and then close the issue 😉

@lionng429
Copy link
Owner

all good :) I tested a bit with the basic example and it should work. 0.4.1 is published as well. thanks once again for checking.

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

No branches or pull requests

2 participants