gm0t / react-sticky-el

MIT License
251 stars 47 forks source link

Create a ref functions to custom components. #22

Closed allanhortle closed 4 years ago

allanhortle commented 6 years ago

Currently if you provide a custom Component to holderCmp or wrapperCmp the ref functions (createHolderRef, createWrapperRef) will return the component not the dom element. This breaks the whole show as things like getBoundingClientRect don't exist.

This PR checks if holderCmp is not a string and then passes the ref down as a prop rather than a ref. Then the custom component is responsible for setting the ref. (Don't know if this is the best test for a custom component. That's all I could think of)

I'm happy to write tests for this, but the test runner seems to be broken on master. Getting this error:

Warning: ReactTestUtils has been moved to react-dom/test-utils. Update references to remove this warning.
module.js:471
    throw err;
    ^

Error: Cannot find module 'react-dom/lib/ReactTestUtils'
    at Function.Module._resolveFilename (module.js:469:15)
    at Function.Module._load (module.js:417:25)
    at Module.require (module.js:497:17)

https://reactjs.org/docs/refs-and-the-dom.html#exposing-dom-refs-to-parent-components

gm0t commented 6 years ago

Hi @allanhortle ! Yeah, tests got broken after upgrading to react@16. But I've just fixed them :) As for the problem, I think it'll be better to fix it using ReactDOM.findDOMNode

  createWrapperRef = (wrapper) => {
    this.wrapperEl = ReactDOM.findDOMNode(wrapper);
  };

  createHolderRef = (holder) => {
    this.holderEl = ReactDOM.findDOMNode(holder);
  };

This should solve the problem with custom components. But I am not sure about Stateless components...

allanhortle commented 6 years ago

Yeah findDOMNode only works with mounted components and doesn't work with functional components.

findDOMNode only works on mounted components (that is, components that have been placed in the DOM). If you try to call this on a component that has not been mounted yet (like calling findDOMNode() in render() on a component that has yet to be created) an exception will be thrown.

findDOMNode cannot be used on functional components.

https://reactjs.org/docs/react-dom.html#finddomnode

It's pretty clunky to write but ref functions seem to be where they are encouraging people. They work with every type of component and have the added advantage of letting the user choose which piece of DOM they want the stuff calculated from.

It's your call though. I don't mind using findDOMNode if you would prefer.

gm0t commented 6 years ago

@allanhortle in this case, may be we should check if wrapper/holder is stateless and in this case give it a holderRef/wrapperRef, otherwise use just ref and findDOMNode. As for "only mounted" case, I don't think that we have such a problem, because we are working only with mounted components. I think this way in 99% of the cases it will work fine and sometimes people will have to use holderRef/wrapperRef. Also, could you please add tests? :)