Fix findDOMNode deprecation warning

[sergio-toro]

This PR fixes React findDOMNode deprecation warning.
https://reactjs.org/docs/strict-mode.html#warning-about-deprecated-finddomnode-usage

This makes sure the component is compatible with upcoming concurrent mode.

It's a breaking change and requires a major release because it adds a node to the DOM, we could use display: contents; as suggested by React.

[joshwnj]

Hi @sergio-toro thanks for this PR.

It all looks good except there seems to be a breaking change related to the containment prop, which you can reproduce if you build the example locally:

• npm run build-example
• open example/index.html in a browser

This may be due to a quirk of the example itself, and I'm actually thinking about deprecating this form of passing a child component, and making the "child function" approach the only supported option, as that would simplify some things.

But please take a look and see if you can see a way to fix it. From what I can see, it might be caused by the extra <div> wrapping the children which wasn't there before.

[sergio-toro]

Hey @joshwnj, thank you for taking time to review the PR!

If we want to stay compliant with concurrent mode the only way is to add the extra node, I tried to avoid it but haven't found any other way, even react team suggests this approach in order to fix findDOMNode deprecation.
The extra div is needed to get the DOM reference, we could use the CSS fix also suggested by react team for this case display: contents;, what do you think?

Did you found issues running the example? Will take a look.

[joshwnj]

Hi @sergio-toro, I believe I've found the reason why the example was failing: because the example uses an absolutely positioned element, when we wrapped it with a <div> it could no longer calculate the position correctly.

I've got a solution for this ready for you to review, could you please grant me access to make changes to this PR? https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork

[sergio-toro]

I've got a solution for this ready for you to review, could you please grant me access to make changes to this PR? https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork

The maintainers check is already enabled in this PR, you should be able to add commits to it!

[joshwnj]

@sergio-toro ah cool, thanks! I've pushed the changes, please take a look and let me know if you think it's ready to merge 👍

[sergio-toro]

@sergio-toro ah cool, thanks! I've pushed the changes, please take a look and let me know if you think it's ready to merge 👍

I'm wondering if we should include the getRef prop. It looks like we're patching it up to solve an issue that can be solved in a neat way changing the API to include a React hook.

I got an idea for it, will do a separate PR with a proposal for a useVisibilitySensor hook alongside the current API.

[sergio-toro]

@joshwnj Check out this proposal PR. #161
What do you think? I believe this goes in the direction React is going.

[MashaDordevic]

It would be great if this is merged and the warning is gone, what's the status?

[ngTurk]

Any updates on that?

[Opty1712]

@joshwnj please