Skip to content
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

Add onPan and onZoom to the PanZoom interaction #3087

Merged
merged 3 commits into from
Jul 21, 2016

Conversation

SamMorrowDrums
Copy link
Contributor

As mentioned in #3086 I am proposing to add events for Pan and Zoom, so that users can automatically retrieve more data for charts.

The proposed API is such:

panZoomInteraction = new Plottable.Interactions.PanZoom();

panZoomInteraction.onPan(callback);
panZoomInteraction.offPan(callback);

panZoomInteraction.onZoom(callback);
panZoomInteraction.offZoom(callback);

Callbacks occur on the most applicable end event (touchend, dragend or scroll), in an attempt to minimise the number of calls where possible, as this will most likely be used for retrieving server data.

@adidahiya
Copy link
Contributor

Nice, this looks clean & well-tested. Is it easy to tell what new data you should retrieve from a server on these events? What might that code look like for Plottable users?

@adidahiya
Copy link
Contributor

Actually, can you change the API to onPanEnd and onZoomEnd? Looking at #2651, it seems like we'd also like to have hooks into the start of these interactions. Naming the ones in this PR more precisely will make it easier to add those hooks later.

// cc @jtlan for thoughts

@adidahiya adidahiya removed the +1 label Jul 14, 2016
@SamMorrowDrums
Copy link
Contributor Author

Yes, that makes sense. I'll update the naming and push again.

I'm actually working on code right now that would fire on this event as we speak! Naturally there are a lot of race conditions that can occur getting async data. Also, there is no actual scroll-end event, so a lot of people implement a timer, and don't fire the true callback until they stop upping the timer.

I'm not sure that the timer code belongs in Plottable codebase though. For now I'll just update the names, and work on implementation of the calling code!

@SamMorrowDrums
Copy link
Contributor Author

Also, for reference I'm using an intermediate callback that gets the x-axis domain for my data gathering:

panZoomInteraction.onZoomEnd(function () {
        const d = that.xScale.domain();
        callback(d[0], d[1]);
});

@jtlan
Copy link
Contributor

jtlan commented Jul 14, 2016

Is it easy to tell what new data you should retrieve from a server on these events? What might that code look like for Plottable users?

I would expect that users would invoke domain() calls to figure out the view window at that time. Alternatively the callbacks could return the domain of the Scales at that time, but there wouldn't be any type information because PanZoom interaction isn't genericized.

* Adds a callback to be called when panning ends.
*
* @param {PanCallback} callback
* @returns {Event} The calling PanZoom Interaction.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and other registration calls should return {this} not {Event}.

@SamMorrowDrums
Copy link
Contributor Author

SamMorrowDrums commented Jul 15, 2016

OK @adidahiya & @jtlan I've fixed the documentation and removed the event arguments from the types, as it sounds like we are not yet decided on best approach for passing the domains (although I agree that is certainly major use case, and users will likely do this manually for now)

I think, if we want a more realistic Zoom event from scroll wheel, we need to implement an intermediate callback that sets a timer (of say 100ms), and keeps cancelling timer and extending until no wheel events occur in time, and callback fires.

Should this work on the event timer be completed now, or are we better progressing with the events, and then building on them (as part of the Zoom and Pan start event work)?

@SamMorrowDrums
Copy link
Contributor Author

SamMorrowDrums commented Jul 15, 2016

I'm doing the timeout in my own code, and its behaviour is pretty solid. Here is roughly what I'm doing in my own codebase:

import Plottable from "plottable";

class PanZoom extends Plottable.Interactions.PanZoom {
    constructor (cb) {
        super();
        this.timer = null;
        this.onDateChange = cb;
        if (cb) {
            this._dragInteraction.onDragEnd(cb);
        }
    }

    _handleWheelEvent (p, e) {
        const that = this;
        super._handleWheelEvent(p, e);
        if (this.onDateChange) {
            if (this.timer) {
                clearTimeout(this.timer);
            }
            this.timer = setTimeout(function () {
                that.onDateChange();
            }, 100);
        }
    }

    _handleTouchEnd (ids, idToPoint, e) {
        super._handleTouchEnd(ids, idToPoint, e);
        if (this.onDateChange && this._touchIds.size() > 0) {
            this.onDateChange();
        }
    }
}

export default PanZoom;

@SamMorrowDrums
Copy link
Contributor Author

SamMorrowDrums commented Jul 15, 2016

@adidahiya you asked how I was planning to implement the calling code, current process is:

  • Callback fires
  • Pass changed domain into an array
  • Check if it's within existing data
  • Send data request (and empty array)
  • Wait until data gathered
  • If more callbacks have fired during data gathering, get the min and max of the domains from callbacks, and then if more data required data, send request (clear array again) and repeat.

@SamMorrowDrums
Copy link
Contributor Author

SamMorrowDrums commented Jul 15, 2016

I have it working perfectly for me, with the data gathering. Really nice feel to scroll eternally through time series and zoom in and out, without any restriction. Obviously, there are some limits, for example you might constrain the array length, and start to pull out data that's no longer visible, but I'm very happy with this behaviour. It really lifts the experience!

@SamMorrowDrums
Copy link
Contributor Author

@adidahiya is there anything further I should do to finish this?

@adidahiya
Copy link
Contributor

@SamMorrowDrums sorry for the delay, this looks great!

@adidahiya adidahiya merged commit dbe07c4 into palantir:develop Jul 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants