-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Annotations offline #2046
Conversation
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
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 solid PR.
I made one blocking comment about window.getComputedStyle
.
src/plots/plots.js
Outdated
@@ -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; |
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.
I'd prefer window.getComputedStyle
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.
... in an effort to make plotly.js work in jsdom
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.
and readability (I think).
if(!msg) msg = ''; | ||
|
||
var path = g.select('path').node(); | ||
expect(getComputedStyle(path).fill).toBe(expectation.bgcolor, msg + ': bgcolor'); |
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.
window.getComputedStyle
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.
... but this is in a test file. I don't care about that as much. Your call.
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.
.toEqual(opacity[i], 'to have correct pt opacity'); | ||
}); | ||
}); | ||
}; | ||
|
||
exports.assertHoverLabelStyle = function(g, expectation, msg, textSelector) { |
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.
🎉
Nicely done 💃 |
Supposed to be a quick Friday night bugfix but it turned a bit more involved:
drawArrowHead
a bit.strict-d3
) that we don't useselection.style
as a getter there or anywhere else.selection.style
as a getter - some others actually could cause bugs of their own (like incolorbar.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