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

Transfer the logic of enabling/disabling pan and zoom to the gesture handler #110

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

asynchaizer
Copy link
Contributor

As part of these changes:

  • Move the logic of enabling/disabling pan and zoom to the gesture handler
  • Add the ability to disable the consumption of gesture events (one of the possible solutions to the problem for Discrete panning in X and Y axes #104)
  • The chart will consume gestures only if necessary
  • Removing the gesture logic "pastTouchSlop", in practice it turned out to be inconsistent when capturing panning
    from the parent container

@asynchaizer asynchaizer force-pushed the feature/refactor_pan_and_zoom_logic branch from 4ac6b8b to 3a52815 Compare January 29, 2025 22:07
Copy link
Member

@gsteckman gsteckman left a comment

Choose a reason for hiding this comment

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

Thanks! This looks really good. The main thing is the one comment for enabling pan consume - it seems to not agree with what the code is doing?

event.consumeChangedPositions()
if (zoomAllowed) {
val centroid = event.calculateCentroid(useCurrent = false)
onZoomChange(size, centroid, ZoomFactor(zoomChange, zoomChange))
Copy link
Member

Choose a reason for hiding this comment

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

Can zoomFactor(zoomChange, zoomChange) be replaced with the val zoom created above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This section will be changed soon, as a panning animation for iOS/Android will be added.
(Actually, the animation implementation is already ready, but unfortunately I can't do cascading PR here, so I'm waiting for the current PR to be accepted)

/**
* Whether the pan gesture on the X-axis should be consumed (handled). Has no effect for `js` and `wasmJs`
* If `true`, the pan gesture will be consumed and will not propagate further
* However, the gesture will still be processed but `PointerInputChange#consume` will not be called,
Copy link
Member

Choose a reason for hiding this comment

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

It looks like from the code, that change.consume() IS being called when this value is true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, you're absolutely right, thanks! :)

@asynchaizer asynchaizer force-pushed the feature/refactor_pan_and_zoom_logic branch from 3a52815 to 5ed060a Compare February 2, 2025 20:02
@asynchaizer asynchaizer requested a review from gsteckman February 3, 2025 13:23
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.

2 participants