-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[charts-pro] Zoom pointer improvements #17480
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
…events-improvement
…events-improvement
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
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.
I will start back the review after eating, but the mobile interaction is far better on my phone with this PR 🚀
packages/x-charts-pro/src/ScatterChartPro/ScatterChartPro.zoom.test.tsx
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.
I played with tooltip and zoom on various charts and everything look nice 🎉👌
| const handlePanStart = () => { | ||
| startRef.current = store.value.zoom.zoomData; | ||
| }; | ||
|
|
||
| const handlePanThrottled = rafThrottle((event: PanEvent) => { | ||
| const target = event.detail.srcEvent.target as SVGElement | undefined; | ||
| if (target?.hasAttribute('data-charts-zoom-slider') || !startRef.current) { | ||
| return; |
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.
Does adding this condition plus an handlePanEnd to clean the startRef.current would avoid the need for 'data-charts-zoom-slider'?
| const handlePanStart = () => { | |
| startRef.current = store.value.zoom.zoomData; | |
| }; | |
| const handlePanThrottled = rafThrottle((event: PanEvent) => { | |
| const target = event.detail.srcEvent.target as SVGElement | undefined; | |
| if (target?.hasAttribute('data-charts-zoom-slider') || !startRef.current) { | |
| return; | |
| const handlePanStart = () => { | |
| if(event is in drawing area){ | |
| startRef.current = store.value.zoom.zoomData; | |
| } | |
| }; | |
| const handlePanThrottled = rafThrottle((event: PanEvent) => { | |
| const target = event.detail.srcEvent.target as SVGElement | undefined; | |
| if (!startRef.current) { | |
| return; |
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.
There is an issue with the releasePointerCapture of the charts cartesian axis which also uses this 'data-charts-zoom-slider'.
🤔
Maybe I can cook up something to replace it. Will look into it.
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've replaced it with the isElementInside check. Let me know waht you think. I can extract it from this PR so it is easier to check
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.
...src/internals/plugins/corePlugins/useChartInteractionListener/useChartInteractionListener.ts
Outdated
Show resolved
Hide resolved
...ternals/plugins/corePlugins/useChartInteractionListener/useChartInteractionListener.types.ts
Outdated
Show resolved
Hide resolved
...ternals/plugins/corePlugins/useChartInteractionListener/useChartInteractionListener.types.ts
Outdated
Show resolved
Hide resolved
| // Clean the interaction when the mouse leaves the chart. | ||
| const moveEndHandler = instance.addInteractionListener('moveEnd', (event) => { | ||
| if (!event.detail.activeGestures.pan) { | ||
| mousePosition.current.isInChart = false; |
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.
While playing with this feature (in the radar docs), keeping track of the pointer position outside of the charts would be nice. Otherwise, it's impossible to see the bottom value (hidden by my finger).
More for a follow up PR
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.
WDYM? When you move the finger to the top of the screen the tooltip moves to the bottom?
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.
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.
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |

Resolves #16205
Fixes #16549
Fixes #16660
Fixes #17480
Supersedes #17000
Difference from the other PR
This uses
@web-gestures/coreto recognise the gestures. I've made the lib from scratch, thinking about extensibility and using the native event emitter api.It should be much lighter and extensible than anything else, though it is new. So bugs might be preset 😆
As I mentioned in one of our previous meeting. It would be nice to have it as part of MUI offering if we deem it a good to have. For now it stays as a separate project.
Main change (same as last PR)
Create a
interaction listenerplugin. This allows us to register events directly in the SVG.The current implementation migrates most of the interactions to the schema below.
Other changes
usePanOnDrag,useZoomOnPinchanduseZoomOnWheelhooks