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

Do not attempt to re-hover on exiting subplots #4269

Merged
merged 4 commits into from
Oct 15, 2019

Conversation

etpinard
Copy link
Contributor

hopefully fixes #4157 and related Cannot read _subplot of undefined issues.

@plotly/plotly_js this bug has been plaguing dash users for some times now (see https://community.plot.ly/t/dash-app-with-js-clientside-callback-sometimes-crashes/29599/5 for a reproducible case).

@etpinard etpinard added bug something broken status: reviewable labels Oct 10, 2019
@etpinard
Copy link
Contributor Author

To include in v1.50.1

Copy link
Contributor

@archmoj archmoj left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me.
💃
@etpinard I only have one question.
Please find my comment below.

src/plots/cartesian/graph_interact.js Show resolved Hide resolved
@alexcjohnson
Copy link
Collaborator

Nice 🎉 presumably this works for disappearing non-cartesian subplots as well as cartesian?

@etpinard
Copy link
Contributor Author

presumably this works for disappearing non-cartesian subplots as well as cartesian?

Presumably, we shouldn't be calling Plots.rehover on non-cartesian subplots ... but hold on looks like mapbox subplots define a _rehover function too:

image

Thanks for the tip!

I'll double-check if something similar happens to mapbox subplots before merging this PR.

@alexcjohnson
Copy link
Collaborator

Presumably, we shouldn't be calling Plots.rehover on non-cartesian subplots

that's not the implication of these comments... anyway regardless of the eventual fix, the test is pretty clear, hover on a subplot of each type while it disappears. Seems a bit overkill for a very simple fix though, also I've never seen users complaining about this with anything but cartesian. But it would be nice to have at least manually tested that this works.

// 'cartesian' case
var plotObj = plots[spId];
if(plotObj) {
supportsCompare = true;
// TODO make sure that fullLayout_plots axis refs
// get updated properly so that we don't have
// to use Axes.getFromId in general.
xaArray[i] = Axes.getFromId(gd, plotObj.xaxis._id);
yaArray[i] = Axes.getFromId(gd, plotObj.yaxis._id);
continue;
}
// other subplot types

@etpinard
Copy link
Contributor Author

@archmoj this PR is ready for a 2nd round of review.

yaArray[i] = Axes.getFromId(gd, plotObj.yaxis._id);
continue;
xaArray[i] = plots[spId].xaxis;
yaArray[i] = plots[spId].yaxis;
Copy link
Contributor Author

@etpinard etpinard Oct 11, 2019

Choose a reason for hiding this comment

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

gd._fullLayout._plots[/* subplot id */].(x|y).axis is now always filled in properly thanks for Plots.linkSubplots called during Plots.supplyDefaults.

That TODO comment above was over three years old.

@archmoj
Copy link
Contributor

archmoj commented Oct 11, 2019

Excellent.
💃 💃

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

Successfully merging this pull request may close these issues.

Cannot read property _subplot of undefined
3 participants