-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Annotation additions: standoff, anchor with arrow, clicktoshow #1265
Conversation
closes #1208 |
Ah good catch - part of the incorrect behavior was already there (while panning, if you start from autoranged, annotations don't get deleted when they go off plot) but the console errors are new. I'll fix both pieces. |
@@ -77,7 +77,7 @@ | |||
"gl-spikes2d": "^1.0.1", | |||
"gl-surface3d": "^1.3.0", | |||
"mapbox-gl": "^0.22.0", | |||
"mouse-change": "^1.1.1", | |||
"mouse-change": "=1.3.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@etpinard @bpostlethwaite temporary change per https://github.com/plotly/streambed/pull/8893
@@ -2093,7 +2093,7 @@ Plotly.update = function update(gd, traceUpdate, layoutUpdate, traces) { | |||
// fill in redraw sequence | |||
var seq = []; | |||
|
|||
if(restyleFlags.fullReplot && relayoutFlags.layoutReplot) { | |||
if(restyleFlags.fullReplot || relayoutFlags.layoutReplot) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@etpinard am I missing something here? Seems like we would have noticed this before if it's a bug, but clicktoshow wouldn't work right without this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. &&
looks correct here.
That if-else-block looks like:
if(restyleFlags.fullReplot && relayoutFlags.layoutReplot) {
// full restyle and relayout update
}
else if(restyleFlags.fullReplot) {
// full restyle update (but not relayout update)
}
else if(relayoutFlags.layoutReplot) {
// full relayout update (but not restyle update)
}
else {
// other more specific updates.
}
Maybe the problem is in layoutReplot
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah OK - thanks, I missed that. I'll look into layoutReplot
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alright, not sure what I was seeing earlier with layoutReplot
/ fullReplot
, but putting back the &&
(b683903) doesn't seem to cause any issues now. Perhaps I fixed it in some other way in the meantime...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
@@ -596,6 +598,13 @@ function hover(gd, evt, subplot) { | |||
|
|||
gd._hoverdata = newhoverdata; | |||
|
|||
// TODO: tagName hack is needed to appease geo.js's hack of using evt.target=true | |||
// we should improve the "fx" API so other plots can use it without these hack. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI @etpinard
@@ -45,7 +45,7 @@ function annAutorange(gd) { | |||
// relative to their anchor points | |||
// use the arrow and the text bg rectangle, | |||
// as the whole anno may include hidden text in its bbox | |||
fullLayout.annotations.forEach(function(ann) { | |||
Lib.filterVisible(fullLayout.annotations).forEach(function(ann) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lib.filterVisible
was unnecessary before because we didn't coerce any other attributes when visible was false, so these annotations wouldn't look like they're on axes. But now I need the position and reference attributes even when annotations aren't visible in order to determine what to show on clicks, hence this filter is needed too.
Added @etpinard @bpostlethwaite ready for review. |
'though, `standoff` is preferred over `xclick` and `yclick`.' | ||
].join(' ') | ||
}, | ||
xclick: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
xclick
and yclick
makes sense in the current state of annotations.
That said, I'm a little afraid about how these attribute will scale when we add data-referenced to other plot types. I'll elaborate on those concerns in more details in #751
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// we should improve the "fx" API so other plots can use it without these hack. | ||
if(evt.target && evt.target.tagName) { | ||
var hasClickToShow = Registry.getComponentMethod('annotations', 'hasClickToShow')(gd, newhoverdata); | ||
overrideCursor(d3.select(evt.target), hasClickToShow ? 'pointer' : ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for pointer
('help'
looks weird to me).
'corresponds to the closest side.' | ||
].join(' ') | ||
}, | ||
clicktoshow: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for clicktoshow
Thanks for that very clear description.
* by moving the name of the original cursor to the data-savedcursor attr. | ||
* omit cursor to revert to the previously set value. | ||
*/ | ||
module.exports = function overrideCursor(el3, csr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to a few test cases for overrideCursor
similar to the ones for setCursor
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha good call - overrideCursor
was actually broken, though somehow it looked like it was working. Fixed & tested in ccbffd0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 🎉
gd = createGraphDiv(); | ||
|
||
// first try to select without adding clicktoshow, both visible and invisible | ||
Plotly.plot(gd, data, layout()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nicely done 🎉
💃 after #1265 (comment) is addressed. |
A few more options for annotations:
xanchor
andyanchor
work along withshowarrow=true
so you can 1) attach the arrow to any edge or corner of the text box, and 2) be sure that regardless of the note size, it will stay away from the point being marked by exactly as many pixels as you specify for the arrow length. One thing to note: the positioning if you rotate the text is different from how it works withshowarrow=false
. Described in Support annotation lines connecting all annotation container faces #1208 (comment):a new attribute was added,
standoff
, which is a number of pixels to move the arrowhead away from the point it's marking. That's particularly useful if you're marking a scatter point with finite size specified in pixels; then you can ensure the arrowhead is always at the edge of the marker regardless of how the plot is zoomed, and the coordinate it is positioned on is exactly the data point you are marking.Also I redid all of the position calculations in a way that I hope will be much clearer for future expansions.
cc @etpinard