-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[charts-pro] Add pan on wheel to zoom
#19998
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
|
Deploy preview: https://deploy-preview-19998--material-ui-x.netlify.app/ Updated pages: Bundle size reportℹ️ Using snapshot from parent commit 789d239 (fallback from merge base d1c95a3).
|
CodSpeed Performance ReportMerging #19998 will not alter performanceComparing Summary
Footnotes |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Signed-off-by: Jose C Quintas Jr <[email protected]>
|
It's a bit hard to showcase, but scrolling on wheel seems to be too slow. When I scroll the page scroll seems much faster than when scrolling the chart. Especially if you use your trackpad and scroll fast, the page will scroll very fast, but the chart won't. Screen.Recording.2025-10-17.at.15.07.21.mov |
packages/x-charts-pro/src/internals/plugins/useChartProZoom/ZoomInteractionConfig.types.ts
Outdated
Show resolved
Hide resolved
alexfauquette
left a comment
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.
About the default config, would it be possible to have by default
- the y-wheel manages the zoom
- the x-wheel manages x-axis pan
That would be nice for people using either track-pad or mouse with a x-wheel
This reverts commit 0cf263a.
| if (!zoomInteractionConfig?.pan) { | ||
| defaultizedConfig.pan = { | ||
| drag: { type: 'drag', requiredKeys: [], mouse: {}, touch: {} }, | ||
| wheel: { type: 'wheel', requiredKeys: [], mouse: {}, touch: {}, allowedDirection: 'x' }, |
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 we should document this limitation, right? Otherwise developers won't realize until their users complain that pan on wheel isn't working properly.
This makes me thing if we should even enable by default if we know it'll have issues.
| return; | ||
| } | ||
|
|
||
| rafThrottledSetZoomData((prev) => { |
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.
Aren't we potentially losing events? rafThrottle will call setZoomData at most once per frame. If we call rafThrottledSetZoomData twice within a frame, the function will be called once per frame with the last arguments.
As a rule of thumb, with rafThrottle we need to accumulate the events and call the throttled function with the accumulated data. If we process the events inside the throttled function we can lose events.
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.
It doesn't really seem to affect wheel events a lot, since they don't need to "sync" with the mouse.
I've added the accumulator regardless
Co-authored-by: Bernardo Belchior <[email protected]> Signed-off-by: Jose C Quintas Jr <[email protected]>
Fixes #13798
Implements: