add support for min/max scale limits

[mojoaxel]

Resolves #5191

This is my first contribution to this repository. Please review this contribution thoroughly! I'm especially not sure if I thought of every edge case of all geo-charts and their parameters.

[mojoaxel]

I could use some help on how to tackle the failed tests in the ci. I tried to run the tests locally but got too many errors.

[mojoaxel]

@archmoj Thanks for reviewing this!

[archmoj]

Now we need to handle

if(maxscale === null) maxscale = Infinity;

or

if(maxscale === -1) maxscale = Infinity;

if we decide to use -1 instead of null which is not a number.
@alexcjohnson your idea on which one is better?

[mojoaxel]

With e722947 I changed the default to -1 instead of null. Scales with less than 0 make no sense here so this should be a valid solution.

[alexcjohnson]

I suppose -1 as a default is OK if we really need a "no max" value - it's better than null which behaves oddly in relayout.

But I wonder if we can't pick a decent default that's more zoomed in than is generally ever useful. I have occasionally gotten in a situation by accidentally scroll zooming so far in that everything freezes up because the SVG engine is having trouble drawing all the off-screen elements. If we cover 99% of cases and avoid this pitfall I'd say that's a reasonable tradeoff, and avoids making a coded value. Something like 10,000?

Similarly, is there a default we could use for minscale, to avoid zooming the whole map away to a point? 0.1 perhaps?

[mojoaxel]

Something like 10,000?
Similarly, is there a default we could use for minscale, to avoid zooming the whole map away to a point? 0.1 perhaps?

Makes sense!
(I'll have a look tomorrow. It's getting late her in europe 😴)

[archmoj]

Shouldn't minscale have a max range of 1?

[mojoaxel]

I don't think so. Maybe it also make sense in some circumstances to only allow scaling between e.g. 2..3.

Then of course the initial scale should also set so one value between 2..3. This check is still missing! What would be a good place to check if scale is within the given min/max bounds and reset it otherwise? Is layout_defaults.js the right place?

[alexcjohnson]

Yes, layout_defaults is the right place - note that coerce returns the set value. Also we should check that minscale <= maxscale

[alexcjohnson]

Also we probably need to check when the scale is automatically determined from the contents of the map whether it's within these bounds, and push it into the range if not. I'm not quite sure where that happens, possibly multiple places in geo.js depending on the situation.

[archmoj]

Right now if I set minscale and maxscaleto 1, I expect the graph not to be scaled. However, usingwheel` scroll zoom the scale changes once.
To fix that you should likely constrain scales in this function as well:

plotly.js/src/plots/geo/zoom.js

Lines 85 to 97 in 5d4c888

	function handleZoom() {
	projection
	.scale(d3.event.scale)
	.translate(d3.event.translate);
	geo.render();
	var center = projection.invert(geo.midPt);
	geo.graphDiv.emit('plotly_relayouting', {
	'geo.projection.scale': projection.scale() / geo.fitScale,
	'geo.center.lon': center[0],
	'geo.center.lat': center[1]
	});
	}

[archmoj]

@mojoaxel we are planning next plotly.js minor (possibly next week) to include new features like this one.
Do you have time to address the remaining comments, so that we could include this feature?

[mojoaxel]

Right now if I set minscale and maxscaleto 1, I expect the graph not to be scaled. However, usingwheel` scroll zoom the scale changes once.

I had a look at this a few days ago. That looks like a d3 issue to me. I could not figure it out.

To fix that you should likely constrain scales in this function as well:

I tried that code and it gets not called at all.

@mojoaxel we are planning next plotly.js minor (possibly next week) to include new features like this one.
Do you have time to address the remaining comments, so that we could include this feature?

Cool, I'll try to look into the open issues today, but I'm not sure that I'll manage to fix the open issues (alone).

@archmoj Feel free to take my code, fix it and create your own pull-request. At the moment I don't have the time to dive deep into this complex codebase.

[nicolaskruchten]

A small update on timing: our team is working hard on releasing v2.0 of Plotly.js, which we anticipate will happen in early April. This PR would be a good candidate to land in the library in v2.1 or later, so with apologies for the delay, we will likely not be able to give much feedback on this PR for the next few weeks :)

[gvwilson]

This pull request has been sitting for a while, so I would like to close it as part of our effort to tidy up our public repositories. I've assigned it to myself to keep track of it; I'll wait until 2024-06-17 for someone to say it's still relevant and they'll to take it on, and otherwise I will close it then. Thanks - @gvwilson

[mojoaxel]

I'll wait until 2024-06-17 for someone to say it's still relevant and they'll to take it on, and otherwise I will close it then.

I still would love to see this merged. I rebased on the current master but I could use some help with the test.

At the moment I would consider this still work-in-progress. I'll try to finish this until 2024-06-17.

[gvwilson]

thanks very much @mojoaxel - much appreciated.