-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Add missing types #3731
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
Add missing types #3731
Conversation
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.
Looks ok. Do we need SingleGestureType
exported?
export { | ||
SingleGestureName, | ||
SingleGestureType, | ||
ComposedGesture as ComposedGestureType, |
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.
Hmm, what do you think about going in the other direction and exporting SingleGesture
, LongPressGesture
, ...?
That will conflict with the API v2, so we may want to rename those and keep that in mind for the migration guide.
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.
Yeah, that would be prefarable, and was my first choice, but I didn't want to conflict with v2 api. If you think it is Ok I will gladly change 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.
Yeah, I think we can rename the old types to something else to free up the "makes sense" names for the future. We would have to include that in the migration guide.
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'll just point out that we should write it somewhere in case we forget. Not sure if doing TODO
comment` is the best, but but we could include it in the roadmap.
|
||
export { SingleGestureName } from './v3/types'; | ||
export { | ||
SingleGestureName, |
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 is that needed?
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.
SingleGestureName
? I'm preety sure it is no longer needed after we added dedicated gesture hooks, am I correct cc @m-bert? I could remove it in this 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.
Yes, I think it is no longer required. It was with useGesture
but now it is only internal (but please make sure that it's true before removing 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.
Removed it in c7a25ef
gestureRelations: GestureRelations; | ||
}; | ||
|
||
export type SingleGestureType = SingleGesture<unknown, unknown>; |
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.
What do you think about a union: TapGestureType | PanGestureType | ...
It would have to be defined elsewhere, though.
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.
Great idea, done in 43bd77d
I figured there is a need for a more general type than handlers corresponding to each gesture, so the user doesn't have to do |
import { FlingGestureEvent, FlingGesture } from './useFling'; | ||
import { HoverGestureEvent, HoverGesture } from './useHover'; | ||
import { LongPressGestureEvent, LongPressGesture } from './useLongPress'; | ||
import { ManualGestureEvent, ManualGesture } from './useManual'; | ||
import { NativeGestureEvent, NativeGesture } from './useNative'; | ||
import { PanGestureEvent, PanGesture } from './usePan'; | ||
import { PinchGestureEvent, PinchGesture } from './usePinch'; | ||
import { RotationGestureEvent, RotationGesture } from './useRotation'; | ||
import { TapGestureEvent, TapGesture } from './useTap'; |
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.
If this imports only types, please use import type
instead of only import
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.
Ok, done in 704d604
export { | ||
SingleGestureName, | ||
SingleGestureType, | ||
ComposedGesture as ComposedGestureType, |
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'll just point out that we should write it somewhere in case we forget. Not sure if doing TODO
comment` is the best, but but we could include it in the roadmap.
export type FlingGestureEvent = | ||
| GestureStateChangeEvent<FlingHandlerData> | ||
| GestureUpdateEvent<FlingHandlerData>; |
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.
Do we actually want to combine these? At the end of the day, those events are accessed in different callbacks
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.
You are right, it is better to separate it. Done in 5113634
|
||
export { SingleGestureName } from './v3/types'; | ||
export type { ComposedGesture } from './v3/types'; | ||
export type { GestureTouchEvent as SingleGestureTouchEvent } from './handlers/gestureHandlerCommon'; |
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 added this reexport to keep naming consistency, but I'm not sure whether this is the best approach. Let me know what you think
export type { TapGestureType } from './handlers/gestures/tapGesture'; | ||
export type { PanGestureType } from './handlers/gestures/panGesture'; | ||
export type { FlingGestureType } from './handlers/gestures/flingGesture'; | ||
export type { LongPressGestureType } from './handlers/gestures/longPressGesture'; | ||
export type { PinchGestureType } from './handlers/gestures/pinchGesture'; | ||
export type { RotationGestureType } from './handlers/gestures/rotationGesture'; | ||
export type { ForceTouchGestureType } from './handlers/gestures/forceTouchGesture'; | ||
export type { ManualGestureType } from './handlers/gestures/manualGesture'; | ||
export type { HoverGestureType } from './handlers/gestures/hoverGesture'; | ||
export type { | ||
ComposedGestureType as ComposedGesture, | ||
RaceGestureType as RaceGesture, | ||
SimultaneousGestureType as SimultaneousGesture, | ||
ExclusiveGestureType as ExclusiveGesture, | ||
ComposedGestureType, | ||
RaceGestureType, | ||
SimultaneousGestureType, | ||
ExclusiveGestureType, |
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.
Not sure about that, TapGestureType
may still be confused with TapGesture
. What about OldTapGesture
/ LegacyTapGesture
?
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.
Ok, changed it to LegacyTapGesture
, though I'm not sure it is the best name.
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.
Legacy*Gesture
sounds fine 😅
export type FlingGestureStateChangeEvent = | ||
GestureStateChangeEvent<FlingHandlerData>; | ||
export type FlingGestureUpdateEvent = GestureUpdateEvent<FlingHandlerData>; |
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.
Maybe we could move it (along with FlingHandlerData
to FlingProperties
file (and probably rename it so it would make sense).
But this one is also fine, if we decide to change it we can do it later 😅
Description
While writing examples I noticed that some types would be useful. This includes types for each gesture and their events. This PR adds those types.
Important: The v3 gesture types take over the names of old v2 types, the v2 types are getting a
Legacy
prefix. This change will have to be added to the migration guide. Added this as a task to roadmap.Test plan
yarn lint-js