-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
add support for min/max scale limits #5192
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
Conversation
071e21b to
e722947
Compare
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.
@archmoj Thanks for reviewing this!
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.
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?
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.
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.
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 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?
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.
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 😴)
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.
Shouldn't minscale have a max range of 1?
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 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?
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.
Yes, layout_defaults is the right place - note that coerce returns the set value. Also we should check that minscale <= maxscale
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.
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.
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
@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?
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.
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 :)
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
1dfbb3a to
bcc3fab
Compare
bcc3fab to
c42cb9d
Compare
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日.
thanks very much @mojoaxel - much appreciated.
Reworked in #7371.
Uh oh!
There was an error while loading. Please reload this page.
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.