search

joshwnj / react-visibility-sensor

Sensor component for React that notifies you when it goes in or out of the window viewport.
MIT License
2.33k stars 195 forks source link

Fix findDOMNode deprecation warning #160

Open sergio-toro opened 5 years ago

sergio-toro commented 5 years ago

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.

image

joshwnj commented 5 years ago

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:

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@joshwnj Check out this proposal PR. https://github.com/joshwnj/react-visibility-sensor/pull/161 What do you think? I believe this goes in the direction React is going.

MashaDordevic commented 3 years ago

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

ngTurk commented 2 years ago

Any updates on that?

Opty1712 commented 1 year ago

@joshwnj please