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

Annotation additions: standoff, anchor with arrow, clicktoshow #1265

Merged
merged 14 commits into from Jan 18, 2017

Conversation

alexcjohnson
Copy link
Contributor

A few more options for annotations:

  • xanchor and yanchor work along with showarrow=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 with showarrow=false. Described in Support annotation lines connecting all annotation container faces #1208 (comment):

    • without an arrow, we first rotate the text box, then calculate the new bounding box and find the appropriate anchor on that. So if you say "left bottom" for the anchor, your note is always above and to the right of the specified coordinates.
    • with an arrow, we attach the specified anchor to the tail of the arrow, and THEN rotate the text box around that anchor point. So now if you say "left bottom" for the anchor, the point that was originally the bottom left corner of the text box stays attached to the arrow tail.
  • 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

@alexcjohnson
Copy link
Contributor Author

closes #1208

@etpinard
Copy link
Contributor

Hmm. Something is off when panning the axes

gifrecord_2016-12-22_110035

@alexcjohnson
Copy link
Contributor Author

Hmm. Something is off when panning the axes

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.

@alexcjohnson alexcjohnson changed the title refactor annotations.draw, add standoff and anchor with arrow Annotation additions: standoff, anchor with arrow, clicktoshow Dec 24, 2016
@@ -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",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -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) {
Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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...

Copy link
Contributor

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.
Copy link
Contributor Author

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) {
Copy link
Contributor Author

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.

@alexcjohnson
Copy link
Contributor Author

alexcjohnson commented Dec 24, 2016

Added clicktoshow #1266 to this PR as it's all somewhat coupled.

@etpinard @bpostlethwaite ready for review.

@etpinard etpinard added this to the v1.22.0 milestone Jan 4, 2017
'though, `standoff` is preferred over `xclick` and `yclick`.'
].join(' ')
},
xclick: {
Copy link
Contributor

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

Copy link
Contributor

Choose a reason for hiding this comment

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

gifrecord_2017-01-16_172046

fun 🎉

// 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' : '');
Copy link
Contributor

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: {
Copy link
Contributor

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) {
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

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())
Copy link
Contributor

Choose a reason for hiding this comment

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

Very nicely done 🎉

@etpinard
Copy link
Contributor

💃 after #1265 (comment) is addressed.

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

Successfully merging this pull request may close these issues.

None yet

2 participants