search

square / maker

Maker Design System by Square
https://square.github.io/maker/styleguide/latest-stable/
Other
63 stars 14 forks source link

fix(popover): fix destroy logic #549

Closed kevinlee11 closed 1 year ago

kevinlee11 commented 1 year ago

Describe the problem this PR addresses

Doing some memory leak testing and looks like popover poppers weren't being properly destroyed resulting in detached HTML elements.

Digging in there were 2 core issues: 1) currentPopper was being accessed in close in the actionApi scope instead of the vm scope, so .destroy() was never being called on the popper instance. 2) Even if 1) was working correctly, the close logic in the actionApi is only triggered if the user clicks the action element (e.g. a button). However, if the user clicks outside to close the popover, the close logic was never called and thus the destroy logic never called (as popperToDestroy is never set)

Describe the changes in this PR

I don't think there was any need for the logic in close for destroying. We can just rely on m-transition-fade-in @after-leave="destroyPopper", because leaving the transition indicates the popover is being closed, whether it's a click on the action element, or an outside click. So get rid of the currentPopper variable and just use popperToDestroy, setting it as before and using it to destroy when the popover fades away.

github-actions[bot] commented 1 year ago

Deployed Styleguide and Lab.

Notes
  1. Links may take a few minutes to update after PR is opened or commit is pushed.
  2. Links may become invalidated after PR is merged or closed.
  3. Links for all releases and open PRs can be found on the Maker Deploys page.

pretzelhammer commented 1 year ago

hey, this introduces a bug, if you quickly click on the show/hide popover button in this PR's styleguide deploy it will wrongly render the popover content inline sometimes:

fast-clicking-mpopover

this doesn't happen if you do it on the latest-stable styleguide

kevinlee11 commented 1 year ago

Ah thanks, good catch!

hey, this introduces a bug, if you quickly click on the show/hide popover button in this PR's styleguide deploy it will wrongly render the popover content inline sometimes:

fast-clicking-mpopover fast-clicking-mpopover

this doesn't happen if you do it on the latest-stable styleguide

github-actions[bot] commented 1 year ago

:tada: This PR is included in version 18.0.8 :tada:

The release is available on:

Your semantic-release bot :package::rocket: