Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Limit zoom bounds when zoomOutRestrict(true) #1026

Closed
wants to merge 2 commits into from

Conversation

indrimuska
Copy link

Problem

Zoom range goes out of bound when I pan the chart over the limits (FIDDLE).
1444905205535screensave

Understanding the problem

That issue occurs because how it's made constrainRange() private function of coordinateGridMixin:

function constrainRange (range, constraint) {
        var constrainedRange = [];
        constrainedRange[0] = d3.max([range[0], constraint[0]]);
        constrainedRange[1] = d3.min([range[1], constraint[1]]);
        return constrainedRange;
}

As you see it doesn't take care if constrainedRange[1] (upper zoom limit) < constrainedRange[0] (lower zoom limit).

Solution

Based on @mhsmith PR for d3.js (d3/d3#1299), I replicated the behavior of the proposed zoom.xExtent method and replaced the function constrainRange() with a new one called restrictZoom().

This is FIDDLE showing the solution using the compiled version of my zoom-limit branch.

@indrimuska
Copy link
Author

Updated (solution) fiddle after last commit:
http://jsfiddle.net/faux1vzy/

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Nov 25, 2016

Fixed references in before-fiddle: https://jsfiddle.net/muxhu0Lh/1/
In after-fiddle: http://jsfiddle.net/gordonwoodhull/7L3cvec4/2/

@gordonwoodhull
Copy link
Contributor

Thanks for the contribution, @indrimuska! This has a whole lot of mapping between domain and range coordinates, and to be honest I've been intimidated to look at it.

But I think it can be much, much simpler. It really just needs to keep the same width (if possible), and clamp to the left and right sides. So I'm going to change the interface a little bit and implement it like this:

    function constrainExtent (extent, constraint) {
        var size = extent[1] - extent[0];
        if (extent[0] < constraint[0]) {
            return [constraint[0], Math.min(constraint[1], constraint[0] + size)];
        } else if (extent[1] > constraint[1]) {
            return [Math.max(constraint[0], constraint[1] - size), constraint[1]];
        } else {
            return null;
        }
    }

Both your implementation and mine have the same flaw that they are not updating the zoom behavior. So you can continue panning off the edge, and internally the zoom behavior will continue to change the translate - and when you start panning back, zoom will be unresponsive until the zoom again corresponds to the chart domain/range.

Still, I think this is better than the old behavior, and d3v4 natively includes the feature, so I think this is good enough for now.

@gordonwoodhull
Copy link
Contributor

I've also taken the liberty of turning your fiddle into an interactive test - I can't imagine how to test this stuff automatically, yet we still do need tests.

My fix and your test will be in dc.js 2.0.

gordonwoodhull added a commit that referenced this pull request Jan 5, 2017
thanks to @indrimuska for the fiddle this was based on

automated tests would be very difficult for this kind of stuff, so we
need tests we can interact with

for #1026
gordonwoodhull added a commit that referenced this pull request Jan 5, 2017
doing regular math on dates results in invalid dates
ref: #1026

this is a little suspicious but at least we are putting the special case
into utils.add/subtract where it will be noticed e.g. for #1230

i'm surprised we haven't needed extent arithmetic before
@indrimuska
Copy link
Author

indrimuska commented Feb 4, 2017

Hi @gordonwoodhull,
I didn't believe that someone could answer to this issue, thank you for this.
In the meantime, I have upgraded my branch in order to support a related bug already present in your interactive test.

You can reproduce it by trying to zoom in one of the bound, let's say on the left side. If you drag the chart on the left nothing happens. But when you try to drag again on the other side (right) it does not change the domain immediately, it does after a while. This is because the "translated domain" has been moved outside the limits and after a while of right-dragging you put it in the right bounds.

I've just pushed a commit in my zoom-limit branch with the fix I found a year ago.
If you like I can open a PR so you can better check the changes.
Thanks :)

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants