magland / figurl-timeseries-views

0 stars 0 forks source link

Timescrollview rollback #4

Closed jsoules closed 1 year ago

jsoules commented 1 year ago

See https://www.figurl.org/f?v=gs://figurl/timeseries-views-1dev&d=sha1://dfb82b18910ba97aa30ef299cc5fd2cd0f9dbe08&label=Timeseries%20graph%20example&s={} for working example.

This PR introduces changes to the TimeScrollView base component to remove dependence on specific times. The immediate impetus is to fix the rollback issue described in Issue #2 but hopefully this will also eliminate some unnecessary re-instantiation of a base function that would drive a lot more rerenders.

Specifically, the ClickReader function in TimeScrollEventHandlers.useClickReader() no longer attempts to return the specific time corresponding to a particular interacted pixel. This reduces rerenders because the click reader had to close over a function that was aware of the start time of the visible window, which changes on panning; instead we only worry about the actual width of the window (which changes less often). (Do note that I have tested and changing the zoom level during an ongoing drag does also work.)

It might be a good idea to impose some rounding on the pixel-to-seconds proportion, to avoid rerenders due to rounding changes as the visible window changes, but I haven't gone there for this PR.

The only other trick is that after every (throttled) drag resolution occurs, we need to update the anchor pixel to the value of the current target pixel. If this is not done, then dragging keeps moving the wrong direction.

As an example, consider a window with 1 pixel per 10 seconds. The user drags from pixel 150 to pixel 250 and then to pixel 200. The first drag would move the window (100 pixels 10 seconds/pixel) = 1000 seconds to the left. Once this window is complete, the user would expect that dragging back to pixel 200 should move the window in the opposite direction, 500 seconds to the right; however if we don't update the anchor. we would compute the second drag as moving (200 - 150 = 50 pixels) to the left*.

Instead, after each render, we update the drag anchor to the original target. This actually aligns perfectly with the intuition of a drag--what the user wants is to take whatever time point is under the anchor and move it to the target. Once that's resolved, we expect the anchor time should actually be under that target pixel, so it makes sense to update the anchor to that new value.

The other changes are minor--adding a dev deploy script and updating a context method to a useCallback rather than a useMemo hook that happened to return a function. (This is more idiomatic but shouldn't have any runtime effect.)

magland commented 1 year ago

I tested the link and it is working much better. Merging.