Adding the ability to specify the tail of an annotation arrow in abso… #610

mdfederici · 2016-06-07T03:32:40Z

…lute point in grid terms rather than relative pixel offset terms. #601

…lute point in grid terms rather than relative pixel offset terms.

…lute point in grid terms rather than relative pixel offset terms. This is useful for the specification of trendlines which will continue to show the correct trend when the chart is zoomed in or out.

# Conflicts: # src/components/annotations/attributes.js

…lute point in grid terms rather than relative pixel offset terms. Squashing commits

…lute point in grid terms rather than relative pixel offset terms.

…to the time I'm spending trying to figure out why they are breaking everything else.

mdfederici · 2016-06-07T04:16:37Z

So it looks like my unit tests were responsible though I have a difficult time understanding how. For now, I've just removed them as they were of dubious value as it was and I don't have time to keep investigating.

If you can quickly see what I was messing up, i'll fix it and add it back.

mdtusz · 2016-06-07T15:34:12Z

You can see the output of the tests here.

One test result - Expected 1088640000000 to equal 1088654400000. looks like it may be a simple case of timezone confusion, but the rest are a bit strange - you can see that there are some failling in axes_test.js with the error Plotly[_module].supplyLayoutDefaults is not a function (where _module would likely be Annotations in that instance. It may be worth testing again on this - especially to cover the date axes scenario.

mdfederici · 2016-06-07T16:48:24Z

Yeah thanks the timezone one i saw and was going to fix if not for the other ones. It was the all of the supplyLayoutDefaults failures which i didn't understand. The diff between them passing and failing was just me removing my unit tests (no source code changes). I'm not a jasmine/plotly expert and I don't understand why my unit tests would cause most of the other tests to fail. Very specifically it was this require:
var Annotations = require('@src/components/annotations');

if i put that in, all of the tests fail and if i remove it, they all pass (except of course the annotation ones). Now I looked at some other tests and tried adding
require('@src/plotly');
which resulted in all of the tests passing including my own. This is odd to me though as it seems like the unit tests aren't isolated. I'll need to look at it more to understand but in the meantime, I've committed this change and I think everything is working.

thanks for helping me with this! I look forward to continuing to contribute.

mdtusz · 2016-06-07T19:20:15Z

Thanks for the contribution!

Regarding the tests - they do run in the same "space" in the browser, so it is possible to have conflicts across files if the tests leak.

This gets a 💃 from me, but I'll wait for @etpinard to give it a pass over before merging it in.

etpinard · 2016-06-13T14:27:06Z

src/components/annotations/attributes.js

+        dflt: false,
+        role: 'style',
+        description: [
+            'Indicates if the tail of this arrow is a point in ',


Wouldn't it be better, more customizable and more plotly.js-esque to have this functionality be set with a pair of attributes, one for each axis?

I'm thinking about:

{ axref: 'x', // or 'pixel' (or 'paper' down the road) similar to 'xref' but for the arrow tail ayref: 'y' }

@mdfederici But this solution might have too many degrees of freedom for most cases. Can you think of a use case where a user would want the x arrow coordinates in pixel and the y in data coordinates (or vice-versa)?

etpinard · 2016-06-13T14:48:02Z

@mdfederici Great PR. This will make a addition to plotly.js.

I would be nice to merge some of several if(absolutetail) you added into a common preprocess step. But that will be for a future PR. Most of the annotation code is pretty ancient and could benefit from a refactor.

The more I think about it the more a like the per-coordinate axref and ayref solution instead of a single absolutetail attribute. I hope you don't mind. It should be pretty easy to patch up.

Regarding the weird jasmine test failure you experiment, annotations/index.js is one of the last remaining files involved in a circular dependency loop - which can lead to bundling errors if not imported in the correct order.

mdfederici · 2016-06-13T16:35:45Z

@etpinard thanks for the review. Regarding the refs, I cannot think of a case where I would use one axis specified in pixels and not the other. In cases like this, I tend to err on the side of YAGNI as long as that path doesn't box me out from implementing said functionality in the future. That being said, you all know your product vision and userbase/use cases much better than I do and i'm happy to implement the refs given that is what you think is right. I'll update the PR later today or tomorrow morning.

I echo your sentiments on the refactor. Several times I found myself wanting to do at least a moderate size refactor but decided against it as I didn't want to take on such a large undertaking due to time constraints (and also didn't want to couple the functionality which I desire to it).

etpinard · 2016-06-13T18:29:43Z

I'll update the PR later today or tomorrow morning.

Thanks!

… Can't figure out how the regex for axis ref works, so hardcoded to x,y for now and seeking guidance.

# Conflicts: # src/components/annotations/attributes.js # src/components/annotations/index.js # test/image/mocks/annotations.json # test/jasmine/tests/annotations_test.js

mdfederici · 2016-06-14T16:05:17Z

@etpinard I've switched to axref and ayref and all seems to be working. You'll see though that I hardcoded the acceptable axis values in attributes.js. This is because I haven't figured out how that regex gets coerced to the correct ref. If i have

ayref: {
    valType: 'enumerated',
    dflt: 'pixel',
    values: [
        'pixel',
        cartesianConstants.idRegex.y.toString()
    ],

...

and I specify: "ayref": "y"

when I coerce for the supplyDefaults, I will get pixel. Any advice for how to do this correctly? thanks.

etpinard · 2016-06-14T16:32:40Z

src/components/annotations/index.js

-            arrowY = Lib.constrain(annPosPx.y - options.ay, 1, fullLayout.height - 1);
+            if(options.axref === options.xref)
+                arrowX = Lib.constrain(annPosPx.x, 1, fullLayout.width - 1);
+            else


🐄 Oh. Please add some { }.

Either as:

if() { } else { }

or

if() { } else { }

But, we allow one-liners like

if() /* */

without braces.

ok no problem. that should be catchable by lint right?

It should. By we haven't decided on a brace-style yet. Making this rule impossible to implement with eslint at the moment.

Personally, I like the Stroustrup style, but most JS folks like the one-trye-brace-style. Moreover, eslint does not have --fix support for that rule yet. So implementing this rule would require some (maybe a lot) of line change.

etpinard · 2016-06-14T16:46:36Z

when I coerce for the supplyDefaults, I will get pixel. Any advice for how to do this correctly?

Ideally, we want the coerce axref using the same logic as xref (and similarly for ayref and yref). Unfortunately, looks like this means refactoring Axes.coerceRef - which isn't the most nicely written function unfortunately 😞

So, I suggest copying much of Axes.coerceRef into a new method called Axes.coerceARef that would handle the axref and ayref and merge them in a later PR (much of axes.js is in dire needs of a refactor already 😄 ).

… duplication of code per github PR 610

mdfederici · 2016-06-14T20:21:29Z

@etpinard i think ready for another review though the copy/paste gives me heartburn :)

mdfederici · 2016-06-14T20:34:47Z

also if this is acceptable, can i convince you to publish to npm or will i need to wait a few weeks?

etpinard · 2016-06-15T14:23:34Z

also if this is acceptable, can i convince you to publish to npm or will i need to wait a few weeks?

The next release 1.14.0 should be made early next week.

etpinard · 2016-06-15T14:24:32Z

src/plots/cartesian/axes.js

+// and coerce it to that list
+axes.coerceARef = function(containerIn, containerOut, gd, axLetter, dflt) {
+    var axlist = gd._fullLayout._has('gl2d') ? [] : axes.listIds(gd, axLetter),
+        refAttr = 'a' + axLetter + 'ref',


nicely done.

mdfederici · 2016-06-15T18:18:12Z

The next release 1.14.0 should be made early next week.

sweet. thanks

etpinard · 2016-06-16T15:45:15Z

@mdfederici I was just making my final review for this PR and I noticed one small bug:

The axref / ayref annotation disappears when panning beyond its (x, y) location on the right.

…was offscreen even if the tail is in coordinate space and on screen.

mdfederici · 2016-06-16T16:50:29Z

@etpinard Good catch thanks. My last commit i think brings it to consistency with the other annotations although I would think that the desired behavior would be to only render the part of the annotation which is within the view (meaning don't draw the half of the arrow which is hanging outside of the graph). I didn't look into implementing that though because i'm unsure if that is how you'd want all annotations to behave and it seems like a big-ish deal change to how annotations draw.

LMK what you think.

etpinard · 2016-06-16T17:18:47Z

My last commit i think brings it to consistency with the other annotations although

Great. That's what I was looking for.

etpinard · 2016-06-17T21:07:09Z

src/components/annotations/index.js

+                        annotationIsOffscreen = true;
+                    }
+
+                    if(annotationIsOffscreen) return;


Hmm. Something isn't quite right still:

The arrow tip gets pinned to the graph's right edge.

@etpinard sigh...its happening because its coming up against the edge of the svg-container. In my testing, that didn't happen before the tail exited the chart so i didn't see it. I'm sorry about that. I believe that I've fixed it and I've added an off screen to the test to ensure it stays that way.

…he chart, rather than render off screen when panned. Added corresponding annotation to image test.

etpinard · 2016-06-20T15:10:35Z

src/components/annotations/index.js

-                arrowY = Lib.constrain(annPosPx.y, 1, fullLayout.height - 1);
+                //we don't want to constrain if the tail is absolute
+                //or the slope (which is meaningful) will change.
+                arrowY = annPosPx.y;


etpinard · 2016-06-20T15:14:29Z

@mdfederici Fantastic PR. Thanks for your efforts 🍻

mdfederici · 2016-06-20T15:18:49Z

@etpinard thanks for bearing with me on this one. the next one will be smoother i think :)

etpinard · 2016-06-20T15:32:29Z

@mdfederici no worries. You did great!

MFed added 7 commits June 3, 2016 08:13

Adding the ability to specify the tail of an annotation arrow in abso…

ed8295a

…lute point in grid terms rather than relative pixel offset terms.

Adding the ability to specify the tail of an annotation arrow in abso…

a2e9c72

…lute point in grid terms rather than relative pixel offset terms. This is useful for the specification of trendlines which will continue to show the correct trend when the chart is zoomed in or out.

Merge remote-tracking branch 'origin/trendline' into trendline

8fcb9dd

# Conflicts: # src/components/annotations/attributes.js

Fixing text and editing.

a7e0e88

code review feedback

722a1ca

Adding the ability to specify the tail of an annotation arrow in abso…

a1da755

…lute point in grid terms rather than relative pixel offset terms. Squashing commits

Adding the ability to specify the tail of an annotation arrow in abso…

a2ecd65

…lute point in grid terms rather than relative pixel offset terms.

mdfederici mentioned this pull request Jun 7, 2016

Support trendline annotations #601

Closed

MFed added 3 commits June 6, 2016 23:51

Merge remote-tracking branch 'origin/trendline' into trendline

8697050

Trying to fix tests for absolutetail.

49d9c7e

Removing these unit tests because they are of dubious value relative …

91dd08a

…to the time I'm spending trying to figure out why they are breaking everything else.

mdtusz added the type: feature label Jun 7, 2016

MFed added 3 commits June 7, 2016 12:11

fixing the unit tests for annotations and adding them back.

dbdcaac

fixing timezone problem in annotations test.

253a3fa

fixing lint for annotation tests.

4aa569d

mdtusz added the status: reviewable label Jun 7, 2016

etpinard reviewed Jun 13, 2016
View reviewed changes

MFed added 4 commits June 14, 2016 11:27

Switching absolutetail to axref, ayref based on code review feedback.…

a008354

… Can't figure out how the regex for axis ref works, so hardcoded to x,y for now and seeking guidance.

Merge remote-tracking branch 'upstream/master' into trendline

925360f

Merge remote-tracking branch 'origin/absolutetail' into absolutetail

3a827a6

# Conflicts: # src/components/annotations/attributes.js # src/components/annotations/index.js # test/image/mocks/annotations.json # test/jasmine/tests/annotations_test.js

somehow I lost this change in my screw up with the branches.

d07b676

etpinard reviewed Jun 14, 2016
View reviewed changes

Not hardcoding axis references but using regex and coerce. Purposeful…

143328d

… duplication of code per github PR 610

etpinard reviewed Jun 15, 2016
View reviewed changes

Fixing bug where annotation would be marked as offscreen if the head …

e83b947

…was offscreen even if the tail is in coordinate space and on screen.

etpinard reviewed Jun 17, 2016
View reviewed changes

Fixing a bug where the x,y of the arrow would be changed to stay on t…

9358a56

…he chart, rather than render off screen when panned. Added corresponding annotation to image test.

etpinard reviewed Jun 20, 2016
View reviewed changes

etpinard merged commit 3c85d9e into plotly:master Jun 20, 2016

mdfederici deleted the absolutetail branch June 20, 2016 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding the ability to specify the tail of an annotation arrow in abso… #610

Adding the ability to specify the tail of an annotation arrow in abso… #610

mdfederici commented Jun 7, 2016 •

edited

mdfederici commented Jun 7, 2016

mdtusz commented Jun 7, 2016

mdfederici commented Jun 7, 2016

mdtusz commented Jun 7, 2016

etpinard Jun 13, 2016

etpinard commented Jun 13, 2016

mdfederici commented Jun 13, 2016

etpinard commented Jun 13, 2016

mdfederici commented Jun 14, 2016

etpinard Jun 14, 2016

mdfederici Jun 14, 2016

etpinard Jun 14, 2016

etpinard commented Jun 14, 2016

mdfederici commented Jun 14, 2016

mdfederici commented Jun 14, 2016

etpinard commented Jun 15, 2016

etpinard Jun 15, 2016

mdfederici commented Jun 15, 2016

etpinard commented Jun 16, 2016 •

edited

mdfederici commented Jun 16, 2016

etpinard commented Jun 16, 2016

etpinard Jun 17, 2016

mdfederici Jun 20, 2016

etpinard Jun 20, 2016

etpinard commented Jun 20, 2016

mdfederici commented Jun 20, 2016

etpinard commented Jun 20, 2016

Adding the ability to specify the tail of an annotation arrow in abso… #610

Adding the ability to specify the tail of an annotation arrow in abso… #610

Conversation

mdfederici commented Jun 7, 2016 • edited

mdfederici commented Jun 7, 2016

mdtusz commented Jun 7, 2016

mdfederici commented Jun 7, 2016

mdtusz commented Jun 7, 2016

Choose a reason for hiding this comment

etpinard commented Jun 13, 2016

mdfederici commented Jun 13, 2016

etpinard commented Jun 13, 2016

mdfederici commented Jun 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Jun 14, 2016

mdfederici commented Jun 14, 2016

mdfederici commented Jun 14, 2016

etpinard commented Jun 15, 2016

Choose a reason for hiding this comment

mdfederici commented Jun 15, 2016

etpinard commented Jun 16, 2016 • edited

mdfederici commented Jun 16, 2016

etpinard commented Jun 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Jun 20, 2016

mdfederici commented Jun 20, 2016

etpinard commented Jun 20, 2016

mdfederici commented Jun 7, 2016 •

edited

etpinard commented Jun 16, 2016 •

edited