adding support for icon-rotate and icon-allow-overlap in mapbox

[zouhairm]

Added ability to specify an angle to rotate symbols in scattermapbox.

This uses the icon-rotate from mapbox. Also added the ability to allow icons to overlap.

Note sure who to tag to review ... Tagging @etpinard since you seem to have authored most of the files I'm modifying.

Question: once this is merged, what's needed to ensure that the plotly.py gets the update? are the attributes.js enough to define the schema for it to be recognized as a valid property? How do I test that by pointing my local version of plotly.py to this branch?

[zouhairm]

I fixed the failing codestyle test.
I am not sure why this is failing though:

     Error: Expected false to be true, 'arrayOk', Object({ valType: 'boolean', dflt: false, role: 'style', arrayOk: false, description: 'Flag to allow symbols to overlap', editType: 'calc' }).

Why would the arrayOk be expect to be true? I understood it as a parameter to signal that this flag should not in fact be allowed to be an array.

[etpinard]

Thanks for the PR!

Why would the arrayOk be expect to be true?

That's because valType: 'boolean' attributes don't support the arrayOk settings. In short,

plotly.js/src/lib/coerce.js

Lines 66 to 74 in 4d24c36

	'boolean': {
	description: 'A boolean (true/false) value.',
	requiredOpts: [],
	otherOpts: ['dflt'],
	coerceFunction: function(v, propOut, dflt) {
	if(v === true || v === false) propOut.set(v);
	else propOut.set(dflt);
	}
	},

doesn't 'arrayOk as part of its otherOpts list. Removing the arrayOk: false line should do the trick.

[etpinard]

Ok thanks @zouhairm for fixing the tests!

I'll tag your PR under the milestone for the next minor (v1.53.0).

Before merging, we'll need to add a few jasmine tests, testing the convert logic, similar to e.g. this one:

plotly.js/test/jasmine/tests/scattermapbox_test.js

Lines 162 to 198 in 0e444b6

	it('should generate correct output for markers + circle bubbles traces', function() {
	var opts = _convert(Lib.extendFlat({}, base, {
	mode: 'markers',
	marker: {
	symbol: 'circle',
	size: [10, 20, null, 10, '10'],
	color: [10, null, '30', 20, 10]
	}
	}));
	assertVisibility(opts, ['none', 'none', 'visible', 'none']);
	expect(opts.circle.paint['circle-color']).toEqual({
	property: 'mcc',
	type: 'identity'
	}, 'circle-color paint');
	expect(opts.circle.paint['circle-radius']).toEqual({
	property: 'mrc',
	type: 'identity'
	}, 'circle-radius paint');
	expect(opts.circle.paint['circle-opacity']).toBe(0.7, 'circle-opacity');
	var circleProps = opts.circle.geojson.features.map(function(f) {
	return f.properties;
	});
	// N.B repeated values have same geojson props
	expect(circleProps).toEqual([
	{ 'mcc': 'rgb(220, 220, 220)', 'mrc': 5 },
	{ 'mcc': '#444', 'mrc': 10 },
	{ 'mcc': 'rgb(178, 10, 28)', 'mrc': 0 },
	{ 'mcc': '#444', 'mrc': 0 },
	{ 'mcc': '#444', 'mrc': 0 }
	], 'geojson feature properties');
	});

and we'll need to update the mapbox_angle.json using orca.

[zouhairm]

I've added a jasmine test for checking angles are set in the features dictionary and allow-icon-overlap flag and icon-rotate property in the symbol.layout.
I've also generated mapbox_angles.png from mapbox_angle.json

LMK if anything else is needed.

BTW, do I need to do anything for this to be accessible by plotly.py, or once it is bumped to point to the new version it will update based on the attributes.js?

[zouhairm]

@etpinard : please let me know if anything else is needed for this or if it can be tagged for the next release?

Also do I need to do something for plotly.py or is that automatic?

[zouhairm]

Hey @etpinard or @archmoj - Any updates on timeline for this?

Alternatively, if you have some input on how I can get this working on my project by using my own fork while waiting for the PR to be merged? I posted a question in the plotly community:
https://community.plot.ly/t/how-to-use-own-fork-of-plotly-js-plotly-py/36412

Thanks!

[archmoj]

@nicolaskruchten in the following weeks, when possible, it would be great to have your thoughts on this PR.
Thank you!

[zouhairm]

Thanks @archmoj - I'm still really hoping for some instructions on how to use my own fork while waiting for this to be merged.

That would be very helpful and motivating for me to continue adding features especially if it can take this long to get a PR merged :|

[nicolaskruchten]

Hi @zouhairm ! I'm really sorry we have not been in a position to review your contribution just yet. We've been very short-handed recently, and we will try to get to it soon.

With respect to Plotly.py, once a PR is merged, the next release of Plotly.js will contain these changes, and the next release of Plotly.py after the JS release will automatically contain the changes. This happens via code-generation in the Plotly.py repository.

It may be possible to run Plotly.py code-generation yourself, based on an un-merged/un-released version of Plotly.js (i.e. your fork) by looking at what https://github.com/plotly/plotly.py/blob/master/contributing.md#update-to-a-new-version-of-plotlyjs does and tweaking some paths to point to your desired version of Plotly.js instead of the official release. If you want to create an issue for this in the Py repo, we can probably help you out and document this more clearly for everyone.

Regarding this pull request, we need to get version 1.53 of Plotly.js out the door this week and then recover a little bit so we will take a look during the 1.54 release cycle. Thanks for your patience and apologies about being radio-silent recently.

[zouhairm]

Thanks @nicolaskruchten - that's unfortunate about the timing, but I was really hoping this would make it in 1.53 after Etienne's comment . I should have stopped by the plotly offices when I was visiting my parents in Montreal back in February :p

I had followed the instructions on updating plotly.js and tried modifying the code but that didn't work and so I posted in the community forums. I'll create an issue on plotly.py with the code I have hopefully get help from there especially if this PR won't be in until 1.54

[nicolaskruchten]

OK. I'm sorry again about the timing, it's been a very tough few weeks for everyone. The good news is that we're expecting the 1.54 cycle to be shorter than usual (2-3 weeks hopefully!), as it mostly contains work that was partly-done in the 1.53 cycle and then deferred so we could get some time-sensitive features shipped.

[alexcjohnson]

This actually seems just fine for symbol and text, since those are both strings / string arrays. But what happens for angles if the user provides an array of numbers-as-strings? A single number as a string will get converted to an actual number by coerce, but for performance we don't reach into arrays to do this (until the calc step - that would be the one benefit of including angle in calc, if it was being used from there here, but I don't recommend that because scattermapbox shares calc with scattergeo so this would need to be a special case.)

It may be that this is just fine - assuming mapbox casts this angle to a number consistently, and treats non-numbers as 0. Can you confirm that this works?

[zouhairm]

ok so I tested this, and yes basically coerce seems to turn a string into an actual number which means it gets interpreted as an acceptable angle, but if provided as an array, then string values get passed on to mapbox which treats non-numbers as 0.

I'm not sure whether that's an acceptable behavior, and honestly I don't think I know enough about the code to figure out how to make it work the way it should without having to have a few back and forths and me diving deeper into other functions. So if you don't mind if this is not acceptable, it'd be great if you could suggest a fix.

[alexcjohnson]

I think 1e1458b should do it 🙏

[alexcjohnson]

@zouhairm this PR is looking great! Just a few small comments above and we should be ready to go. Apologies again for the delay getting this looked at.

[zouhairm]

I am not sure why this started failing as of f9eb48b - relative to 6943d7a the only change was in a string ...

unless test-jasmine3 is a new test? I'll try rebasing on master just in case ...

[alexcjohnson]

Alright, aside from me breaking and fixing your test with the explicit 0, looks like the tests are just a bit flaky, took a little rerunning to pass. Some days CI just does that...

Looks great! 💃 - @archmoj I'll let you merge when you're ready.

[archmoj]

Using dflt zero has broken this mock.
The markers on the persepective view (larger view) should not have any titlt/rotation.

To preserve current behaviour the dflt for angle should be set to null.
I'll open a PR to address this problem.

[alexcjohnson]

heh OK, that was my instigation :) I'm not sure a simple null is exactly what we want but I'll look at your fixup PR now @archmoj