-
-
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
Enable text for scattergl trace #2737
Conversation
Nice @dy 🎉 Anything in particular you'd like me to help out with? |
src/traces/scattergl/convert.js
Outdated
textOptions.align = 'left'; | ||
break; | ||
case 'center': | ||
case 'centre': |
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.
That's not a valid value. We should implement the same textposition
values as scatter:
'top left', 'top center', 'top right',
'middle left', 'middle center', 'middle right',
'bottom left', 'bottom center', 'bottom right'
@dy regl-text appears pretty slow at 3000 Try: Plotly.d3.json('https://plot.ly/~jackp/18395.json', (err, fig) => {
fig.data[1].type = 'scattergl'
fig.config = {scrollZoom: true}
console.time('regl-text')
Plotly.newPlot(gd, fig)
console.timeEnd('regl-text')
}) I get 5000-6000ms |
}, | ||
unselected: { | ||
marker: scatterAttrs.unselected.marker | ||
marker: scatterAttrs.unselected.marker, | ||
textfont: scatterAttrs.unselected.textfont |
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.
Does this work? And what about selected.textfont
?
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.
Confirmed. selected.textfont.color
and unselected.textfont.color
do not work.
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.
mostly done in 1a64005
src/components/drawing/index.js
Outdated
@@ -622,8 +624,7 @@ function textPointPosition(s, textPosition, fontSize, markerRadius) { | |||
|
|||
var numLines = (svgTextUtils.lineCount(s) - 1) * LINE_SPACING + 1; | |||
var dx = TEXTOFFSETSIGN[h] * r; | |||
var dy = fontSize * 0.75 + TEXTOFFSETSIGN[v] * r + | |||
(TEXTOFFSETSIGN[v] - 1) * numLines * fontSize / 2; | |||
var dy = fontSize * 0.75 + TEXTOFFSETSIGN[v] * r + (TEXTOFFSETSIGN[v] - 1) * numLines * fontSize / 2; |
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.
Can you try not committing things like that in the future? Thank you.
mode: { | ||
valType: 'flaglist', | ||
flags: ['lines', 'markers'], | ||
flags: ['lines', 'markers', 'text'], |
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.
Now that scattergl supports on-graph, we can add hovertext
like in svg scatter traces:
plotly.js/src/traces/scatter/attributes.js
Lines 91 to 104 in 6ac47ec
hovertext: { | |
valType: 'string', | |
role: 'info', | |
dflt: '', | |
arrayOk: true, | |
editType: 'style', | |
description: [ | |
'Sets hover text elements associated with each (x,y) pair.', | |
'If a single string, the same string appears over', | |
'all the data points.', | |
'If an array of string, the items are mapped in order to the', | |
'this trace\'s (x,y) coordinates.', | |
'To be seen, trace `hoverinfo` must contain a *text* flag.' | |
].join(' ') |
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.
done in 959a555
@@ -213,6 +220,11 @@ function sceneUpdate(gd, subplot) { | |||
if(scene.line2d) scene.line2d.update(opts); | |||
if(scene.error2d) scene.error2d.update(opts.concat(opts)); | |||
if(scene.select2d) scene.select2d.update(opts); | |||
if(scene.glText) { |
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.
Can you 🔪 this out?
@@ -161,6 +165,7 @@ function sceneOptions(gd, subplot, trace, positions, x, y) { | |||
return opts; | |||
} | |||
|
|||
|
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.
Again here. Please don't commit lines like this.
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.
Ok, but in this case all functions are separated by 2 new lines and here by just one, so hope you don't mind a bit of perfectionism
@@ -87,7 +87,10 @@ function updateHeadersInSrcFiles() { | |||
}); | |||
|
|||
function isCorrect(header) { | |||
return (header.value === licenseStr); | |||
return ( | |||
header.value.replace(/\s+$/gm, '') === |
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.
Is this for windows compat?
If so, I would prefer a different solution. Trimming spaces here could lead to false positive assertions.
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.
never mind. Thanks for the fix!
TODO:
|
@@ -0,0 +1,144 @@ | |||
{ | |||
"data": [ |
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.
gl2d mocks use the gl2d_
prefix
@dy would you mind renaming these new mocks?
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.
Fixed
Ok, should be fixed! |
Thanks @dy ! Things have improved as far as I can tell on High Sierra on browserstack. The in Chrome 67: in Safari 11.1: but something is still off in FF 61: |
@etpinard hmm, I am getting |
@etpinard can you please try one more time? (blind debugging, but no choice) |
@dy for |
@etpinard is that persistent behavior? |
what do mean by permanent? First render is a given tabs is incorrect, hitting refresh once or mulitple times give the correct view |
Hopefully @alexcjohnson will have time to try this out in the next 15 hours or so. If not, I'm tempted to just merge this thing, flag it as "beta" in the release notes and have users try it out so that we can accumulate reports. Thoughts? N.B. I'm about to go offline until about 9am tomorrow EDT (no internet at home after moving day). |
Definite improvement, though there are still some oddities - I'm seeing similar effects to what @etpinard reported. I'd be OK with merging this as is and considering these issues bugs. Fairly urgent bugs though! On Mac OS 10.13.5, retina display. For reference here's the SVG |
Merging as "beta". Please comment on #2780 if you're seeing odd behavior. |
Some benchmarks using: var n = /**/;
var letters = 'abcdefghijklmnopqrstuvwxyz';
var text = [];
var x = [];
var y = [];
for(var i = 0; i < n; i++) {
x.push(Math.random())
y.push(Math.random())
text.push(letters[i % letters.length])
}
console.time('tx')
Plotly.newPlot(gd, [{
type: 'scattergl', // or 'scatter'
mode: 'text',
x: x,
y: y,
text: text,
}])
console.timeEnd('tx')
But note that scattergl does not cluster text nodes, so panning and selection at when n > 1e5 is slow. |
Implements #2501
gl_text_chart_*
mocks