-
Notifications
You must be signed in to change notification settings - Fork 148
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
Gesture Options Cleanup #696
Gesture Options Cleanup #696
Conversation
/// A constant factor that determines how quickly pan deceleration animations happen. | ||
/// Multiplied with the velocity vector once per millisecond during deceleration animations. | ||
/// Defaults to `UIScrollView.DecelerationRate.normal.rawValue` | ||
public var panDecelerationFactor: CGFloat = UIScrollView.DecelerationRate.normal.rawValue |
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.
everywhere else in the code, this has been called decelerationFactor. Why is it that the options have a prepended pan in front of 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.
We want to indicate that this setting only applies to pan. In the other locations, the meaning is clear from the context. Later, we might add other separate deceleration factor options for other gestures, so the prefix sets us up to be able to do that in an additive way.
case doubleTapToZoomIn | ||
|
||
/// The double touch to zoom out gesture | ||
case doubleTouchToZoomOut |
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.
nit: Let's order these alphabetically
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.
Can we debate this in a separate ticket?
self.initialTouchLocation = nil | ||
self.initialCameraState = nil | ||
self.lastChangedDate = nil |
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.
Should we make this completion optional? Seems like it should be nullable instead of sending an empty closure here.
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.
let's address this in a future pr.
ad05bdb
to
5fdc54d
Compare
c40d056
to
6409cca
Compare
* remove hapticFeedbackEnabled (#663) * remove hapticFeedbackEnabled` * clean up changelog * Make gestureOptions.decelarationRate the source of truth (#662) * making gesture options decelaration rate the source of truth * clean up changelog * fix indentation * Internalize UIGestureRecognizerDelegate (#669) * seperated gesture recognizer delegate into a new class and fixed downstream breaks * adding changelog * swift lint and code clean up * adding a comment * pr comments * Refactor gestures (#677) #### Breaking Changes - Pan deceleration has been temporarily removed - `TapGestureHandler.init` was previously public by mistake and is now internal - The behavior of `GestureManager.options` has been updated to better reflect the `isEnabled` state of the associated gesture recognizers - The gesture recognizer properties of `GestureManager` are no longer `Optional` - `GestureType` has been redesigned so that its cases have a 1-1 relationship with the built-in gestures #### Public API Additions - `CameraState`'s fields are now `var`s instead of `let`s for testing purposes, and a public, memberwise initializer has been added. - `PanScrollingMode` now conforms to `CaseIterable` - `GestureType` now conforms to `CaseIterable` #### Bug fixes - GestureManager no longer sets itself as the delegate of all gestures in MapView when its options change #### Internal Refactoring - Generalizes `CameraAnimatorMapboxMap` by renaming it to `MapboxMapProtocol` so that it can be used throughout the SDK. SDK seems small enough that per-component dependency inversion feels unnecessary and might negatively impact binary size. - Removes old `GestureHandlerDelegate` by injecting `MapboxMap` and `CameraAnimationsManager` into each gesture handler to allow each handler to manipulate the camera directly. - Removes unnecessary `GestureContextProvider` - Refactors handlers and `GestureManager` for dependency injection and more thorough tests - Tidies up handlers and `GestureManager` and associated tests for greater consistency * Pan Deceleration (#692) * update changelog * fix changelog * Gesture Options Cleanup (#696) * wip * Change double tap to zoom out to "double touch" * Fix warning * Lint * Update changelog * [run device tests] * Use explicit self Co-authored-by: Andrew Hershberger <andrew.hershberger@mapbox.com> Co-authored-by: Nishant Karajgikar <nishant.karajgikar@mapbox.com>
Pull request checklist:
Summary of changes
Breaking changes
GestureManager.rotationGestureRecognizer
has been removed. Rotation is now handled by.pinchGestureRecognizer
in addition to its preexisting handling of panning and zooming.GestureManager.doubleTapToZoomOutGestureRecognizer
has been replaced with.doubleTouchToZoomOutGestureRecognizer
PanScrollingMode
has been renamed toPanMode
GestureOptions.zoomEnabled
has been replaced by.doubleTapToZoomInEnabled
,.doubleTouchToZoomOutEnabled
, and.quickZoomEnabled
GestureOptions.rotateEnabled
has been removedGestureOptions.scrollEnabled
has been renamed to.panEnabled
GestureOptions.scrollingMode
has been renamed to.panMode
GestureOptions.decelerationRate
has been renamed to.panDecelerationFactor
GestureType.doubleTapToZoomOut
has been replaced with.doubleTouchToZoomOut
GestureType.rotate
has been removedGestureType
cases have been reordered for consistency withGestureOptions
andGestureManager
Bug Fixes