-
-
Notifications
You must be signed in to change notification settings - Fork 1k
[Web] Adapt NativeViewGestureHandler
to NativeDetector
#3653
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
base: @akwasniewski/native-detector-web
Are you sure you want to change the base?
[Web] Adapt NativeViewGestureHandler
to NativeDetector
#3653
Conversation
95c800c
to
c00cdcf
Compare
…ative-gesture-web
c00cdcf
to
2a1d8a7
Compare
packages/react-native-gesture-handler/src/v3/HostGestureDetector.web.tsx
Outdated
Show resolved
Hide resolved
packages/react-native-gesture-handler/src/v3/HostGestureDetector.web.tsx
Outdated
Show resolved
Hide resolved
packages/react-native-gesture-handler/src/v3/HostGestureDetector.web.tsx
Show resolved
Hide resolved
packages/react-native-gesture-handler/src/web/tools/GestureHandlerWebDelegate.ts
Outdated
Show resolved
Hide resolved
const attachTarget = shouldAttachToChild | ||
? childRef.current | ||
: viewRef.current; |
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.
const attachTarget = shouldAttachToChild | |
? childRef.current | |
: viewRef.current; | |
const attachTarget = shouldAttachToChild | |
? viewRef.current.firstChild | |
: viewRef.current; |
I think viewRef.current
is of type HTMLDivElement
in web in this case, so we should be able to read the child (we may want to assert there is one).
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! Tried to figure out the solution without wrapping, but I did not know that on web we could just specify the view to be a div. Fixed in 1f473cd
389716c
to
13ab34c
Compare
@@ -13,13 +12,22 @@ export interface GestureHandlerDetectorProps extends PropsRef { | |||
const HostGestureDetector = (props: GestureHandlerDetectorProps) => { | |||
const { handlerTags, children } = props; | |||
|
|||
const viewRef = useRef(null); | |||
const viewRef = useRef<HTMLDivElement>(null); |
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.
In theory it can also be SVGElement
. Would it be ok to use just Element
as type?
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.
As @j-piasecki suggested I used HTMLDivElement as only it has firstChild
attribute and view on web is a div anyway. It allowed me to get rid of the wrapping. If it really can be somethng different than a div I might have to go back to wrapping. Is there a use case for an SVG element to have a native gesture? Maybe I can only typecast there, when accessing firstChild.
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 I can only typecast there, when accessing firstChild.
If firstChild
doesn't exist then type cast won't help - it won't work anyway 🤷
If it really can be something different than a div I might have to go back to wrapping.
As far as I remember svg
tags inherit from SVGElement
, you can check that 😅
Is there a use case for an SVG element to have a native gesture?
Probably not, this configuration doesn't sound reasonable to me (cc @j-piasecki)
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 firstChild doesn't exist then type cast won't help - it won't work anyway
Yes, but if native gesture could only be on divs I could possibly check the view type in
const shouldAttachGestureToChildView = (handlerTag: number): boolean => {
return RNGestureHandlerModule.getGestureHandlerNode(
handlerTag
).shouldAttachGestureToChildView() && viewRef.current instanceof HTMLDivElement;
};
and then the type cast would be safe
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.
Then you could probably use type guard and type cast wouldn't be necessary
@@ -54,9 +75,9 @@ const HostGestureDetector = (props: GestureHandlerDetectorProps) => { | |||
}, []); | |||
|
|||
return ( | |||
<View style={{ display: 'contents' }} ref={viewRef}> | |||
<div style={{ display: 'contents' }} ref={viewRef}> |
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 know it works, but why do you switch to div
?
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.
Explained in #3653 (comment)
Description
Following #3638 this PR brings support for
NativeViewGestureHandler
intoNativeDetector
for web. It does so, by attaching handler into child instead of detector.Test plan
Tested on the following code