-
-
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
Implementation of arrayOk textposition for scatter3d traces #3200
Conversation
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.
Nice PR @archmoj - thanks!
The changes in gl-vis/gl-scatter3d#11 also look good. I made two small comments here.
As this is a new feature, we'll hold off on merging it for at least one week - in case we need to make another patch under 1.42.x
src/traces/scatter3d/attributes.js
Outdated
@@ -160,7 +160,7 @@ var attrs = module.exports = overrideAll({ | |||
colorAttributes('marker') | |||
), | |||
|
|||
textposition: extendFlat({}, scatterAttrs.textposition, {dflt: 'top center', arrayOk: false}), | |||
textposition: extendFlat({}, scatterAttrs.textposition, {dflt: 'top center', arrayOk: true}), |
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.
Making this line
textposition: extendFlat({}, scatterAttrs.textposition, {dflt: 'top center'}),
would suffice as textposition
in the scatter attributes is already arrayOk
:
plotly.js/src/traces/scatter/attributes.js
Lines 520 to 535 in fe2d8d7
textposition: { | |
valType: 'enumerated', | |
values: [ | |
'top left', 'top center', 'top right', | |
'middle left', 'middle center', 'middle right', | |
'bottom left', 'bottom center', 'bottom right' | |
], | |
dflt: 'middle center', | |
arrayOk: true, | |
role: 'style', | |
editType: 'calc', | |
description: [ | |
'Sets the positions of the `text` elements', | |
'with respects to the (x,y) coordinates.' | |
].join(' ') | |
}, |
src/traces/scatter3d/convert.js
Outdated
@@ -133,14 +133,49 @@ function calculateErrorParams(errors) { | |||
return {capSize: capSize, color: color, lineWidth: lineWidth}; | |||
} | |||
|
|||
function parseAlignmentX(a) { | |||
if(a === null || a === undefined) return 0; | |||
else if(typeof(a) === 'number') return a; |
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.
Why would textposition[i]
ever be a number 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.
Well, I thought we could somehow make use of other numbers (instead than only -1|0|1) to scale the distances. But yeah I may remove those condition checks (also for null & undefined) since I noticed the inputs seem to be parsed and validated before getting to there?
"type": "scatter3d", | ||
"mode":"lines+markers+text", | ||
"text": ["middle center", "bottom", "right", "bottom right", "bottom left", "left", "top right", "top", "top left"], | ||
"textposition": ["", "bottom", "right", "bottom right", "bottom left", "left", "top right", "top", "top left"] |
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.
Nice mock!
src/traces/scatter3d/convert.js
Outdated
@@ -233,7 +266,7 @@ function convertPlotlyOptions(scene, data) { | |||
} | |||
|
|||
if('textposition' in data) { | |||
params.textOffset = calculateTextOffset(data.textposition); // arrayOk === false | |||
params.textOffset = calculateTextOffset(data.textposition, data.dflt); |
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.
Hmm. What's data.dflt
here?
@archmoj I'm trying to plot Plotly.newPlot(gd, [{
type: 'scatter3d',
mode: 'markers+text',
x: [1, 2],
y: [1, 2],
z: [1, 2],
text: ['a', 'b'],
textposition: [null, undefined]
}]) off this branch, but I'm getting: which points to: plotly.js/src/plot_api/helpers.js Lines 265 to 274 in f84af74
so we might have to patch that |
src/plot_api/helpers.js
Outdated
|
||
if(textposition.indexOf('left') !== -1) posX = 'left'; | ||
else if(textposition.indexOf('right') !== -1) posX = 'right'; | ||
if(textposition !== undefined && textposition !== null) { |
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 think we have to be even more strict at this stage here. Anything that's not a string shouldn't fall into this block. The items inside textposition
can really be anything.
Have you tried something like:
Plotly.newPlot(gd, [{
type: 'scatter3d',
mode: 'markers+text',
x: [1, 2],
y: [1, 2],
z: [1, 2],
text: ['a', 'b'],
textposition: [null, undefined, true, false, [], {}, NaN, Infinity]
}])
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 would you mind adding a jasmine test for this
Somewhere at the end of this describe block would be 👌
This PR is ✅ You can go ahead and merge gl-vis/gl-scatter3d#11 , release |
This PR resolves #2872 so that the arrays could be used for different alignments of texts in scatter3d.
@etpinard
@alexcjohnson