Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ContextMenu] clean up click event listener for setCanBeClosed() #8274

Closed
1 of 2 tasks
tay1orjones opened this issue Apr 1, 2021 · 2 comments
Closed
1 of 2 tasks

Comments

@tay1orjones
Copy link
Member

tay1orjones commented Apr 1, 2021

What package(s) are you using?

  • carbon-components
  • carbon-components-react

Detailed description

Follow up issue from #8099 - in browsers that aren't Safari, the click event listener should be removed in the useEffect cleanup function.

In theory the garbage collector will prevent this (small) leak but it would be best to ensure it's removed when the component is unmounted.

You can see in chrome that multiple event listeners are registered when the ContextMenu is opened a few times.
image

FYI @janhassel something I missed in review 🙃
Thanks for taking a second look at this with me @andreancardona !

@janhassel
Copy link
Member

Thanks for catching that, @tay1orjones and @andreancardona !

I just wanted to look further into this but I'm not sure if I understand exactly: the two additional event listeners (3 and 4) come from the internal ClickListener component and listen for an outside click to close the menu. Each ContextMenu is wrapped inside one of those and since the demo has a nested menu / two levels, there are two ClickListeners. For comparison: if you go to the "Multiple groups" story with only one level, you'll only see one of those event listeners.
I couldn't see any listeners piling up when opening and closing the menu a bunch of times.

Let me know if I'm missing something

@andreancardona
Copy link
Contributor

@janhassel going to close this issue - @tay1orjones doesn't see it, and I don't see it - they must have been 👻 event listeners

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants