-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[charts-pro] Add tapAndDrag
zoom gesture
#19727
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-19727--material-ui-x.netlify.app/ Updated pages: Bundle size report
|
CodSpeed Instrumentation Performance ReportMerging #19727 will not alter performanceComparing Summary
Footnotes |
|
||
- **`wheel`**: Zoom in/out by scrolling the mouse wheel (default) | ||
- **`pinch`**: Zoom in/out by pinching on touch devices (default) | ||
- **`tapAndDrag`**: Zoom in/out by tapping once and then dragging vertically. Dragging up zooms in, dragging down zooms out. |
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 think of this a "double tap and drag", since you tap once, then you lift the finger and tap a second time before dragging.
So, "doubleTapAndDrag" would better describe the action, and some people do call it "double tap and drag" (here and here, here).
The only place I saw it called "tap-and-drag" was here, but the description then mentions a double tap:
Tap-and-drag is performed by double tapping anywhere on the display.
So, we can either:
- Rename it to
doubleTapAndDrag
or, - Update the description here to say "[...] by tapping twice [...]"
I'm fine either way.
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.
thats odd, I wouldn't consider it a double-tap because our tap interaction is defined as a down+up
interaction, and in this case it is down+up/down+move
, in my understanding a double tap would be down+up/down+up/down+move
. We don't say the drag
is a tap and drag
interaction.
I'm ok with changing if you still think that should be the case
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.
We don't say the drag is a tap and drag interaction.
True. I agree there is some inconsistency.
I think we can keep the name, but I'd update the description to say "tapping twice". Maybe even call out "similar to Google/Apple Maps"? What do you think?
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.
updated
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.
@bernardobelchior added a prevent default for touchstart
when we detect the tap. Safari has this behaviour by default when you taptap
, and there is not a lot of ways to get rid of it.
If we always prevent the default, then the touch-action: pan-y
wouldn't work. So we add/remove this listener at the same stage we used to add/remove the touchAction
changes I did a few commits ago
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's now working perfectly, great job!
I have a fundamental question about this gesture/feature. Since this is only applicable for touch screen devices, why would someone do this when they can pinch to zoom and drag to pan. These feel more intuitive to me. Also, does Google maps support this? I couldn't replicate it on my phone. |
Yes, Apple Maps as well. |
Got it, yes just checked it both on apple and google maps. I have never used this gesture, but ok this seems to be there for accessibility. Also, as Bernardo pointed out, they describe it as "double tap and drag". |
packages/x-charts-pro/src/internals/plugins/useChartProZoom/gestureHooks/useZoomOnTapAndDrag.ts
Show resolved
Hide resolved
packages/x-internal-gestures/src/core/gestures/TapAndDragGesture.ts
Outdated
Show resolved
Hide resolved
packages/x-charts-pro/src/internals/plugins/useChartProZoom/gestureHooks/useZoomOnTapAndDrag.ts
Outdated
Show resolved
Hide resolved
packages/x-internal-gestures/src/core/gestures/TapAndDragGesture.ts
Outdated
Show resolved
Hide resolved
packages/x-internal-gestures/src/core/gestures/TapAndDragGesture.test.ts
Outdated
Show resolved
Hide resolved
packages/x-internal-gestures/src/core/gestures/TapAndDragGesture.test.ts
Outdated
Show resolved
Hide resolved
packages/x-internal-gestures/src/core/gestures/TapAndDragGesture.test.ts
Show resolved
Hide resolved
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 interaction is looking great! Nice job 💯
tapAndDrag
zoom gesturetapAndDrag
zoom gesture
Fixes #18955
I went with a custom
TapAndDragGesture
rather than relying on theTapGesture
andPanGesture
directly, as they would interfere with other behaviours. (Eg: if user is panning but thetapAndDrag
changed the drag config, the original pan would stop working)Internally we are instantiating a new
TapGesture
and a newPanGesture
and augmenting their behaviour, rather than reimplementing/copy-pasting the tap and drag behaviours in a single gesture.https://deploy-preview-19727--material-ui-x.netlify.app/x/react-charts/zoom-and-pan/#interactions
Changelog
tapAndDrag
gesture. Zoom in/out by tapping twice and dragging vertically.