Skip to content

Remove all event listeners from gd #117

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

Merged
merged 2 commits into from
Dec 17, 2015
Merged

Remove all event listeners from gd #117

merged 2 commits into from
Dec 17, 2015

Conversation

mdtusz
Copy link
Contributor

@mdtusz mdtusz commented Dec 15, 2015

Piece of 🍰

@mdtusz mdtusz mentioned this pull request Dec 15, 2015
@@ -941,6 +941,7 @@ Plotly.redraw = function(gd) {
Plotly.newPlot = function (gd, data, layout, config) {
gd = getGraphDiv(gd);
plots.purge(gd);
gd.removeAllListeners();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd would put this in plots.purge

and maybe (cc @alexcjohnson ) we could expose purge on Plotly? It sounds to me like something users might want to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call on moving removeAllListeners. Not sure if we should expose the purge method though - I can't think of too many use cases where that's what a user would want to clear the node and reuse it - if they want to "purge", they'll just delete the node. Better to keep the user surface area of plotly.js as small as possible.

@alexcjohnson thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mdtusz

if they want to "purge", they'll just delete the node.

You got me convinced 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

purge it all

@alexcjohnson
Copy link
Collaborator

I think there are still some client installations that use plots.purge because they were written before we had Plotly.newPlot... but yeah, I'm good with hiding plots.purge anyway and if those old clients ever upgrade plotly.js they can deal, it's probably only 2 or 3 of them. - if you don't follow purge up with another Plotly.plot all hell breaks loose.

@emackey
Copy link

emackey commented Dec 17, 2015

I'd like to use something similar to purge or destroy to get rid of a plot that is being dismissed from the UI of a single-page web app. The plot may have been in a popup or overlay, and that's going away now, so I want to kill off the render loop, the event handlers, and indeed get rid of any complex DOM structure that is no longer needed.

@etpinard
Copy link
Contributor

💃

mdtusz added a commit that referenced this pull request Dec 17, 2015
Remove all event listeners from gd
@mdtusz mdtusz merged commit b2f07f7 into master Dec 17, 2015
@mdtusz mdtusz deleted the unbind-click-events branch December 17, 2015 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants