Generalize hover picking routine #575
Conversation
- don't assume that fullLayout._plots is defined
- don't assume that fullLayout._plots[''] is defined
- use fullLayout[spId]._subplot.{x,y}axis as subplot x/y axes
in non-cartesian cases
- perf: replace two .map calls with one for loop
| return Axes.getFromId(gd, spId, 'y'); | ||
| }), | ||
| hovermode = evt.hovermode || fullLayout.hovermode; | ||
| plots = fullLayout._plots || [], |
There was a problem hiding this comment.
IMPORTANT keep fullLayout._plots for cartesian subplots (and gl2d although I'm having second doubt about this) only.
There was a problem hiding this comment.
Why do we need to keep them separate? In my mind, this seems like it will just introduce more surface area for inconsistent state bugs.
There was a problem hiding this comment.
Mostly for convenience.
Most of our per-subplot code post-supply-defaults is designed as:
- loop over all subplots of a given type found
fullLayout - create a subplot object or update an existing subplot object in
fullLayout[/* name of subplot */]._subplot
So, it becomes convenient to store a ref to the subplot object in its corresponding fullLayout attribute container.
Moreover, as finding all xy subplots in a given figure is somewhat expensive (see Axes.getSubplots), it becomes advantages to look for them in fullLayout._plots directly instead of calling Axes.getSubplots every time we need to loop over all the x/y subplots.
|
Looks good to me. 💃 |
- looks like fullLayout._plots does not get updated properly on resize. Fixing that deserves a PR of its own. - this commit reverts to the pre-#575 behavior
Another PR in preparation for our mapbox-gl integration.
In brief, this PR generalizes the
Fx.hoveringraph_interact.js, so that it can usexaxisandyaxisfrom arbitrary subplot instance.At the moment, only
cartesianandternarysubplots useFx.hoverto handle their hover labels. But with after this PR, makinggeoandmapboxsubplots useFx.hoverwill be a piece of 🍰 .