Implementation of arrayOk textposition for scatter3d traces

[archmoj]

This PR resolves #2872 so that the arrays could be used for different alignments of texts in scatter3d.
@etpinard
@alexcjohnson

[etpinard]

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

[etpinard]

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(' ')
	},

[etpinard]

Why would textposition[i] ever be a number here?

[archmoj]

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?

[etpinard]

Nice mock!

[etpinard]

Hmm. What's data.dflt here?

[etpinard]

@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

	if(!Registry.traceIs(trace, 'pie') && !Registry.traceIs(trace, 'bar')) {
	if(Array.isArray(trace.textposition)) {
	for(i = 0; i < trace.textposition.length; i++) {
	trace.textposition[i] = cleanTextPosition(trace.textposition[i]);
	}
	}
	else if(trace.textposition) {
	trace.textposition = cleanTextPosition(trace.textposition);
	}
	}

so we might have to patch that cleanTextPosition helper.

[etpinard]

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]
}])

[etpinard]

... and would you mind adding a jasmine test for this ⤴️

Somewhere at the end of this describe block would be 👌

[etpinard]

This PR is ✅

You can go ahead and merge gl-vis/gl-scatter3d#11 , release gl-scatter3d patch. Looks like you'll also need to merge a conflict in gl3d_plot_interact. Once that's done 💃