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

Timeline zooming and panning #1006

Merged
merged 13 commits into from
Dec 6, 2023
Merged

Timeline zooming and panning #1006

merged 13 commits into from
Dec 6, 2023

Conversation

AaronPlave
Copy link
Contributor

@AaronPlave AaronPlave commented Nov 14, 2023

Implements temporal zooming and panning using d3-zoom. Hold command (mac) or control (windows) and scroll on any timeline row to initiate zooming. Use this same key and click and drag to pan. Middle mouse button can also be used instead of the modifier key. This functionality also applies to the x-axis time area as well as the histogram. The histogram currently does not require the user to hold down the modifier key but this needs discussion. Smooth zoom/pan performance is also noticeably worse with simulations that contain a large number of points but this should be addressed separately by general line plot rendering improvements.

Closes #986, see that issue for more commentary.

TODO:

  • Test on windows
  • Prevent interaction conflicts between dragging activities and panning

@AaronPlave AaronPlave changed the title Timeline zooming and panning WIP Timeline zooming and panning Nov 14, 2023
@AaronPlave AaronPlave marked this pull request as ready for review November 27, 2023 19:04
@AaronPlave AaronPlave requested a review from a team as a code owner November 27, 2023 19:04
@AaronPlave AaronPlave changed the title WIP Timeline zooming and panning Timeline zooming and panning Nov 27, 2023
Copy link
Collaborator

@duranb duranb left a comment

Choose a reason for hiding this comment

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

I'm good with this! It's muuuch easier to get to anywhere on the timeline. Also, the tooltip taking a shortcut change 🥇.

I'm also good with getting this merged in prior to testing on Windows. If we can get this deployed on dev, then it might be more easily accessible for users with Windows to test out.

Copy link
Collaborator

@dandelany dandelany left a comment

Choose a reason for hiding this comment

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

Tested the features noted in the #986 checklist and everything looks good, I think this is good to go! Thanks @AaronPlave. Two very small "take it or leave it" notes -

  • I think the current behavior is "If Mac, use Cmd key as modifier, else use Control". I wouldn't mind if both keys worked on both systems, to make it easier for people who use both, since we're unlikely to override the other cmd/ctrl key for any other uses.
  • I did have a hiccup while running this at first - after npm install and npm run dev I got the error below & had to rm -rf node_modules & reinstall to get it to run. Possibly related to the package-lock changes, not sure if anyone else will hit this.

@duranb
Copy link
Collaborator

duranb commented Dec 6, 2023

  • I think the current behavior is "If Mac, use Cmd key as modifier, else use Control". I wouldn't mind if both keys worked on both systems, to make it easier for people who use both, since we're unlikely to override the other cmd/ctrl key for any other uses.

The key that's the spatial equivalent to CMD on a windows keyboard is ALT, which I'm not sure is something we want to have to maintain parity with because the functional Mac equivalent to the ALT key is the Option key (which is a different key spatially on Mac). I think that check might be more complicated than it's worth, but there might be a cleaner way that I'm not thinking of. I'm ok with doing a CMD || CTRL check, since the code already checks the two keys if Aaron decides to.

@AaronPlave
Copy link
Contributor Author

  • I think the current behavior is "If Mac, use Cmd key as modifier, else use Control". I wouldn't mind if both keys worked on both systems, to make it easier for people who use both, since we're unlikely to override the other cmd/ctrl key for any other uses.

The key that's the spatial equivalent to CMD on a windows keyboard is ALT, which I'm not sure is something we want to have to maintain parity with because the functional Mac equivalent to the ALT key is the Option key (which is a different key spatially on Mac). I think that check might be more complicated than it's worth, but there might be a cleaner way that I'm not thinking of. I'm ok with doing a CMD || CTRL check, since the code already checks the two keys if Aaron decides to.

Ctrl + click on Mac opens the context menu so don't think that'd work, not something we'd want to override. I think for now I'm in the camp of let it be a single key for each OS and if it proves to be difficult for people to remember then we can perhaps find a different modifier key?

@AaronPlave AaronPlave merged commit 0c93d68 into develop Dec 6, 2023
4 checks passed
@AaronPlave AaronPlave deleted the feat/zooming-panning branch December 6, 2023 16:29
JosephVolosin pushed a commit that referenced this pull request Aug 20, 2024
* Add d3-zoom
* Zoom and pan around timeline, histogram, and x-axis using command/control on mac/windows
* Prevent layer activity mouse down interactions if meta/ctrl is pressed
* Remove temporary unlock on timeline lock control
* Implement TimelineInteractionMode control button
* Use computed tooltip params when updating tooltip props
* Fix for previously shown tooltips disappearing after menu open or hover
* Use tooltip shortcut props for ValueSourceBadge
JosephVolosin pushed a commit that referenced this pull request Oct 21, 2024
* Add d3-zoom
* Zoom and pan around timeline, histogram, and x-axis using command/control on mac/windows
* Prevent layer activity mouse down interactions if meta/ctrl is pressed
* Remove temporary unlock on timeline lock control
* Implement TimelineInteractionMode control button
* Use computed tooltip params when updating tooltip props
* Fix for previously shown tooltips disappearing after menu open or hover
* Use tooltip shortcut props for ValueSourceBadge
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.

Zoom/pan with scroll gestures using modifier keys
3 participants