tajo / react-portal

🎯 React component for transportation of modals, lightboxes, loading bars... to document.body or else.
MIT License
2.14k stars 169 forks source link

Fixed Portal content not being removed on toggle in React < 16 #199

Closed shahankit closed 6 years ago

shahankit commented 6 years ago

Removes portal content using ReactDOM.unmountComponentAtNode for React < 16.

tajo commented 6 years ago

Thanks!

bjoerge commented 6 years ago

Any reason to not call ReactDOM.unmountComponentAtNode(this.defaultNode) before removing it too? I'm still having the issue described in #198 when not passing a node.

shahankit commented 6 years ago

Hi @bjoerge when you unmount Portal, the portal content are still visible? I tried replicating that in examples but couldn't get it to work. I also added a test case and it seems to work fine in my case:

ifReact(
  '< 16',
  test,
  test.skip
)('should remove portal content for default node', () => {
  document.body.innerHTML = '<div id="root"></div>';
  ReactDOM.render(
    <Portal>
      <div>Foo</div>
    </Portal>,
    document.getElementById('root')
  );
  ReactDOM.unmountComponentAtNode(document.getElementById('root'));
  expect(document.getElementById('root').innerHTML).toBe('');
});

I think a better approach would be to call ReactDOM.unmountComponentAtNode first and then call document.body.removeChild. This way the lifecycle hooks of Portal content will also be called.

componentWillUnmount() {
  ReactDOM.unmountComponentAtNode(this.defaultNode || this.props.node);
  if (this.defaultNode) {
    document.body.removeChild(this.defaultNode);
  }
  this.defaultNode = null;
  this.portal = null;
}

cc @tajo

bjoerge commented 6 years ago

Hi @shahankit! So my issue is not that the portal is still visible, but that componentWillUnmount is never called on its child nodes (preventing me from doing cleanup tasks like removing listeners, etc).

I think your suggested approach here makes sense and will solve it 👍