-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[charts-pro] Allow zoom interactions to be configured #18646
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
[charts-pro] Allow zoom interactions to be configured #18646
Conversation
Deploy preview: https://deploy-preview-18646--material-ui-x.netlify.app/ Updated pages: Bundle size report
|
CodSpeed Performance ReportMerging #18646 will not alter performanceComparing Summary
|
packages/x-charts-pro/src/internals/plugins/useChartProZoom/gestureHooks/usePanOnDrag.ts
Outdated
Show resolved
Hide resolved
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
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.
The API and the code structure look good. Just had one question
This pull request has been inactive for 30 days. Please remove the stale label or leave a comment to keep it open. Otherwise, it will be closed in 15 days. |
…figuration-simplified
packages/x-charts-pro/src/internals/plugins/useChartProZoom/ZoomConfig.types.ts
Outdated
Show resolved
Hide resolved
packages/x-charts-pro/src/internals/plugins/useChartProZoom/ZoomConfig.types.ts
Outdated
Show resolved
Hide resolved
…figuration-simplified
…figuration-simplified
This reverts commit 8be2e5b.
packages/x-charts-pro/src/internals/plugins/useChartProZoom/useChartProZoom.ts
Outdated
Show resolved
Hide resolved
export type AddInteractionListener = { | ||
<CustomData extends Record<string, unknown> = Record<string, unknown>>( | ||
interaction: 'pan' | 'panStart' | 'panEnd', | ||
interaction: 'pan' | 'panStart' | 'panEnd' | 'zoomPan' | 'zoomPanStart' | 'zoomPanEnd', |
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 don't get what are doing those new interactions. What's the difference between a pan
and a zoomPan
?
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 divided them because the gesture settings are now configurable on the gesture level, this is so the zoom configuration doesn't affect tooltip behaviour.
packages/x-charts-pro/src/internals/plugins/useChartProZoom/gestureHooks/usePanOnDrag.ts
Outdated
Show resolved
Hide resolved
packages/x-charts-pro/src/internals/plugins/useChartProZoom/gestureHooks/useZoomOnPinch.ts
Outdated
Show resolved
Hide resolved
packages/x-charts-pro/src/internals/plugins/useChartProZoom/gestureHooks/useZoomOnWheel.ts
Outdated
Show resolved
Hide resolved
Nice fix 👍 |
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.
Minor curiosity, otherwise looks good 👍
const onZoomChange = useEventCallback(onZoomChangeProp ?? (() => {})); | ||
const optionsLookup = useSelector(store, selectorChartZoomOptionsLookup); | ||
|
||
useEffectAfterFirstRender(() => { |
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.
Why do we want this use effect to only run after the first render? What happens if it runs in the first render as well?
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 hook synchronise the props in the store.
Every time a user update a props saved in the store, we need to update it.
But we don't need that at the first run because the store is already in sync thanks to the plugin's getInitialState()
So we skip the first run of the useEffect
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.
Thanks for clarifying!
It would probably be useful to have this explanation in the code 😄
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.
🚀
Allow configuring with simple strings for accepting defaults or with full configuration.
Goal is to allow a more fine-grained customisation of the gestures, while also making the PR that adds more supported gestures smaller :)
Changelog