alexkatz / react-tiny-popover

A simple and highly customizable popover react higher order component with no other dependencies! Typescript friendly.
MIT License
454 stars 121 forks source link

Elements remain in React tree #27

Closed kityan closed 4 years ago

kityan commented 5 years ago

Hello. What may be the reason that elements generated by Popover remain in react tree after popover close? They are not in DOM, but in React tree (visible in React Dev Tools). Is it a kind of leakage? Is it a problem with portals?

You can try with the original demo: https://alexkatz.github.io/react-tiny-popover/ Open/close popover many times and you will find many ArrowContainer in tree IMHO all window listeners and timer interval are removed on destroy, so what is the reason?

kityan commented 5 years ago

Anyway, even w/o React Dev Tools you can find elements, appended by Popover in Detached HTMLDivElement branch of Google Chrome DevTools Memory Snapshot.

frfroes commented 5 years ago

+1 to this, we're having the same issue. Besides the memory leak of having several components mounted it also makes hard to trigger behaviors that should happen inside componentWillUnmount.

mattaningram commented 5 years ago

I am also having this issue, is this repo still being maintained?

tjfocare commented 5 years ago

+1

tonix-tuft commented 5 years ago

Hi folks, does anyone know if there's a way to maintain the popup's DOM element in the DOM after closing the popup? If the popup has complex content inside of it, then if the user closes and reopens the popup, React has to rebuild the whole component's tree, and if this process is expensive, the user may wait several seconds before the popover reappears.

It would be great if the rendered popover remains in the DOM after being hidden, but I was wondering whether react-tiny-popover provides such functionality or not.

calummoore commented 5 years ago

Can you not just set the css to be display: none?

tonix-tuft commented 5 years ago

@calummoore That seems to be the only way to do it. The thing is, the popover does not implement ref forwarding so, e.g., you cannot use:

<Popover ref={this.popoverRef}
    ...
>
    ...
</Popover>

And access the popover's div within this.popoverRef.current (assuming you created the ref with this.popoverRef = React.createRef).

Therefore, the only solution I can think of would use the containerClassName prop and access the popover in the parent component through plain Vanilla JS code, for instance:

<Popover
    containerClassName="my-custom-popover-class"
    ...
>
    ...
</Popover>

And access it with:

const popover = document.querySelector('.my-custom-popover-class')

Or, you can use the containerStyle prop, it would be even easier...

tonix-tuft commented 5 years ago

I would also like to use opacity: 0 instead of just display: none in order to trigger the transition. Handle such behaviour is a bit more cumbersome as the popover does not expose a hidden event where I can set display: none after the transition ends...

calummoore commented 5 years ago

@tonix-tuft The opacity transition is done using css, and so there's no way to hook for that. The best alternative is to use setTimeout for the duration of the animation, and then set to display: none when it runs.

Yeah containerStyle would be the easiest way.

alexkatz commented 4 years ago

I believe this issue has been resolved in the latest release. I'm unable to reproduce it. If I'm mistaken, I apologize. I'll close this now, but feel free to reference this in a new issue if the problem persists.