Skip to content

Annotations offline #2046

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

Merged
merged 8 commits into from
Oct 2, 2017
Merged

Annotations offline #2046

merged 8 commits into from
Oct 2, 2017

Conversation

alexcjohnson
Copy link
Collaborator

Supposed to be a quick Friday night bugfix but it turned a bit more involved:

  • Fixes The annotation arrow isn't rendered properly on load #992 (annotations not styling correctly if the plot is off-DOM when it's first rendered).
  • Updates the style of drawArrowHead a bit.
  • Tests (via strict-d3) that we don't use selection.style as a getter there or anywhere else.
  • Gets rid of all the other uses of selection.style as a getter - some others actually could cause bugs of their own (like in colorbar.draw), some were benign (if they dealt with eg hover labels so we know the element is displayed) but it's easier - and in some cases simpler code even - to just be consistent in not using it.

cc @etpinard

at one point I envisioned this as a more general routine, to add heads to any line
and we could still do that, but other uses will just need to follow the same naming.
instead of hard-coding these in drawArrowHead
Copy link
Contributor

@etpinard etpinard left a comment

Choose a reason for hiding this comment

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

Very solid PR.

I made one blocking comment about window.getComputedStyle.

@@ -212,7 +212,12 @@ plots.redrawText = function(gd) {
plots.resize = function(gd) {
return new Promise(function(resolve, reject) {

if(!gd || d3.select(gd).style('display') === 'none') {
function isHidden(gd) {
var display = getComputedStyle(gd).display;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer window.getComputedStyle here.

Copy link
Contributor

Choose a reason for hiding this comment

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

... in an effort to make plotly.js work in jsdom

Copy link
Contributor

Choose a reason for hiding this comment

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

and readability (I think).

if(!msg) msg = '';

var path = g.select('path').node();
expect(getComputedStyle(path).fill).toBe(expectation.bgcolor, msg + ': bgcolor');
Copy link
Contributor

Choose a reason for hiding this comment

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

window.getComputedStyle

Copy link
Contributor

Choose a reason for hiding this comment

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

... but this is in a test file. I don't care about that as much. Your call.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated them all -> 78f5222

tested that we only call getComputedStyle on window and only as many times as expected -> 97cea60

.toEqual(opacity[i], 'to have correct pt opacity');
});
});
};

exports.assertHoverLabelStyle = function(g, expectation, msg, textSelector) {
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

@etpinard
Copy link
Contributor

etpinard commented Oct 2, 2017

Nicely done 💃

@alexcjohnson alexcjohnson merged commit 8b35a8e into master Oct 2, 2017
@alexcjohnson alexcjohnson deleted the annotations-offline branch October 2, 2017 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The annotation arrow isn't rendered properly on load
2 participants