Uh oh!
There was an error while loading. Please reload this page.
Add spikelines - #2247
Conversation
…ta will not be overwritten
etpinard
commented
Jan 15, 2018
I'll review this tomorrow am (UTC-5). Sorry for the wait. |
etpinard
left a comment
There was a problem hiding this comment.
@apalchys amazing work!
To the best of my knowledge, this latest iteration has everything @alexcjohnson described in his #2155 (comment) done in a very clear and concise matter.
I found a few ways to make your additions to fx/hover.js even clearer. Let me know what you think!
My most blocking comments are the following:
- declare
hoverdistanceandspikedistancewithmin: -1 - clicking on spikes button an even number of times in the modebar should get the user back to the original settings
Just as an FYI, if you can addressed these issues before tomorrow night, this PR will be merged and released as part of 1.33.0 on Thursday.
| hoverdistance: { | ||
| valType: 'integer', | ||
| min: 0, |
There was a problem hiding this comment.
As Infinity isn't JSON-serializable, we declare attributes of the likes (e.g. hoverlabel.namelength) with min: -1, so that users can set hoverdistance: -1 for an infinite search radius. Morevover, hoverdistance: 0 should imply no hover labels/events, similarly for spikedistance.
| role: 'style', | ||
| editType: 'none', | ||
| description: [ | ||
| 'Sets the default distance (in points) to look for data', |
There was a problem hiding this comment.
We usually write (in pixels) instead of (in points) in our attribute descriptions.
| else hovermodeDflt = 'closest'; | ||
| coerce('hovermode', hovermodeDflt); | ||
| coerce('hoverdistance'); |
There was a problem hiding this comment.
We could get fancy here by making this:
varhoverMode=coerce('hovermode',hovermodeDflt);if(hoverDistance)coerce('hoverdistance');as hoverdistance is useless when hovermode: false.
Unfortunately, we can't add similar logic for spikedistance here as showspikes is per axis.
There was a problem hiding this comment.
Actually, we can, because if hovermode is false, we are not firing any hover events, so the spikelines are disabled too.
So we can write it like this:
varhoverMode=coerce('hovermode',hovermodeDflt);if(hoverMode){coerce('hoverdistance');coerce('spikedistance');}| fullLayout._cartesianSpikesEnabled = fullLayout.hovermode === 'closest' ? | ||
| (fullLayout._cartesianSpikesEnabled === 'on' ? 'off' : 'on') : 'on'; | ||
| fullLayout._cartesianSpikesEnabled = fullLayout._cartesianSpikesEnabled === 'on' ? 'off' : 'on'; |
There was a problem hiding this comment.
We might still need something similar here.
For example, with
varlayout={xaxis: {showspikes: true},yaxis: {showspikes: false}};then click on toggleSpikeLines gives:
with no way to get back to the original behavior using the mode bar buttons.
There was a problem hiding this comment.
Great. Thanks!
Now, could you restore and update the modebar_test.js cases you removed in your first commit?
| bottom: pos, | ||
| left: ax._offset, | ||
| rigth: ax._offset + ax._length, | ||
| right: ax._offset + ax._length, |
There was a problem hiding this comment.
Good catch. Did this cause any bugs previously?
There was a problem hiding this comment.
It did in one of my early implementations
| return result; | ||
| } | ||
| if(fullLayout._has('cartesian')) { |
There was a problem hiding this comment.
Nice use of _has('cartesian') 👌
| } | ||
| function spikesChanged(gd, oldspikepoints) { | ||
| // don't emit any events if nothing changed |
There was a problem hiding this comment.
Looks like this got copied from function hoverChanged() {}, but spike lines don't emit any events whatsoever. Would you mind 🔪 this line to avoid confusion.
There was a problem hiding this comment.
I use it to avoid the redrawing of the plot because of new spikelines if spikelines points didn't change
| if(closestVPoints.length) { | ||
| var closestVPt = closestVPoints[0]; | ||
| if(isNumeric(closestVPt.x0) && isNumeric(closestVPt.y0)) { | ||
| tmpPoint = clearClosestPoint(closestVPt); |
There was a problem hiding this comment.
This clearClosestPoint method is doing mostly initialisation as opposed to clearing something that exists already.
Would be mind rename it either initClosestPoint or fillClosestPoint? This would make it more aligned with the rest of our codebase.
There was a problem hiding this comment.
You are right, I've renamed it to fillClosestPoint.
| xpx = xa.c2p(xval), | ||
| ypx = ya.c2p(yval), | ||
| dxy = function(point) { | ||
| var rad = Math.max(3, point.mrc || 0), |
There was a problem hiding this comment.
This Math.max(3, point.mrc || 0) only applies to scatter traces and is a duplicate of this line in scatter/hover.js.
Perhaps we should make the relevant traces/*/hover.js methods (I can think of scatter at the moment, but there might be more) add a .kink field to their output point data to not have to duplicate the somewhat tricky logic here again?
There was a problem hiding this comment.
Moved it to the point.kink property of the scatter/hover returned object.
| var xa, | ||
| ya; | ||
| var showY = closestPoints.hLinePoint ? true : false; |
There was a problem hiding this comment.
maybe:
varshowY=!!closestPoints.hLinePoint;// orvarshowY=Boolean(closestPoints.hLinePoint);would suffice?
There was a problem hiding this comment.
Thanks, changed it to var showY = !!closestPoints.hLinePoint;
apalchys
commented
Jan 17, 2018
@etpinard Thank you for the review! I fixed all highlighted issues. Please check. |
apalchys
commented
Jan 17, 2018
hmm.. some test is failing but I can't reproduce it locally. Is it possible to re-run tests without commits? |
alexcjohnson
commented
Jan 17, 2018
Mmm, I think I've seen that one fail sporadically. You can't rerun the test, but we can... |
etpinard
commented
Jan 17, 2018
Awesome @apalchys Once the spike toggling modebar tests are restored and updated (#2247 (comment)), this PR will be good to merge! |
etpinard
commented
Jan 17, 2018
Great PR. Thanks very much @apalchys 💃 💃 💃 |
apalchys
commented
Jan 17, 2018
@etpinard@alexcjohnson Thank you for your help! 👍 |
lambone
commented
Feb 11, 2018
I found it does not work for scattergl and because the pointData in function hoverPoints lacks of kink attribute. |
alexcjohnson
commented
Feb 11, 2018
I checked the version 1.34.0, it does not work yet. I made a simple fix in src/traces/scattergl/index.js. |
alexcjohnson
commented
Feb 23, 2018
@wangyueming#2379 was only merged 3 days ago. It will be in 1.35, likely next week. |
This PR adds support of spikelines in any hovermode.
Also some new properties were added:
spikedistanceandhoverdistanceto control the range of appearing of spikelines and hoverlabels;spikesnapto control the placement of the spikelines point.Note: only the cartesian type of axis supports new functionality.
Relates to #2155