plotly.js icon indicating copy to clipboard operation
plotly.js copied to clipboard

add support for min/max scale limits

Open mojoaxel opened this issue 5 years ago • 9 comments
trafficstars

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 avatar Oct 05 '20 15:10 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 avatar Oct 07 '20 08:10 mojoaxel

@archmoj Thanks for reviewing this!

mojoaxel avatar Oct 26 '20 19:10 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. To fix that you should likely constrain scales in this function as well: https://github.com/plotly/plotly.js/blob/5d4c888407306f2719c76c67d79f8203bcdaac1a/src/plots/geo/zoom.js#L85-L97

archmoj avatar Oct 27 '20 14:10 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?

archmoj avatar Nov 04 '20 00:11 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.

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.

mojoaxel avatar Nov 04 '20 19:11 mojoaxel

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 :)

nicolaskruchten avatar Mar 10 '21 21:03 nicolaskruchten

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

gvwilson avatar Jun 10 '24 17:06 gvwilson

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.

mojoaxel avatar Jun 10 '24 23:06 mojoaxel

thanks very much @mojoaxel - much appreciated.

gvwilson avatar Jun 12 '24 14:06 gvwilson