Skip to content

[Web] Native detector component #3637

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

Open
wants to merge 53 commits into
base: next
Choose a base branch
from

Conversation

akwasniewski
Copy link
Contributor

Description

This PR adds a web version of NativeDetector component following its IOS/android implementations.

Test plan

  • copy all react source files from basic-example to expo-example
  • add a console log, in the NativeDetector pan Gesture
  • check whether the gesture is properly recognised

@m-bert m-bert requested a review from j-piasecki July 25, 2025 10:18
@akwasniewski akwasniewski requested a review from j-piasecki July 28, 2025 08:51
@akwasniewski akwasniewski requested a review from j-piasecki July 29, 2025 09:01
@akwasniewski akwasniewski marked this pull request as ready for review July 29, 2025 09:18
@akwasniewski akwasniewski requested a review from m-bert July 29, 2025 09:40
@akwasniewski akwasniewski requested a review from j-piasecki August 1, 2025 10:35
@akwasniewski akwasniewski force-pushed the @akwasniewski/native-detector-web branch from bfac6a5 to 06a3c86 Compare August 4, 2025 08:30
@akwasniewski akwasniewski force-pushed the @akwasniewski/native-detector-web branch from 06a3c86 to 9c17a96 Compare August 4, 2025 08:31
@akwasniewski akwasniewski force-pushed the @akwasniewski/native-detector-web branch from e1ef94a to f3198f5 Compare August 5, 2025 08:18
Copy link
Contributor

@m-bert m-bert left a comment

Choose a reason for hiding this comment

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

I can see that you updated some handlers implementations, what about Pan, `Tap, etc. don't they have to be updated too?

@@ -21,8 +22,8 @@ interface DefaultViewStyles {
export class GestureHandlerWebDelegate
implements GestureHandlerDelegate<HTMLElement, IGestureHandler>
{
private isInitialized = false;
private _view!: HTMLElement;
public isInitialized = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like making this public, can we make a getter to it (preferably with name better than getIsInitialized)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made private with a getter called initialized in 1f62d5c

@akwasniewski
Copy link
Contributor Author

akwasniewski commented Aug 5, 2025

I can see that you updated some handlers implementations, what about Pan, `Tap, etc. don't they have to be updated too?

I only needed to update those handlers than override the init function, because we pass additional props. Those using parent implementation don't need to be directly updated

Comment on lines +48 to +52
if (this.delegate.initialized) {
// this function is called on handler creation, which happens before initializing delegate
const view = this.delegate.view as HTMLElement;
this.restoreViewStyles(view);
}
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this break the native gesture somewhat? If the view isn't set when init is called, restoreViewStyles wouldn't be called at all, no?

Copy link
Contributor Author

@akwasniewski akwasniewski Aug 6, 2025

Choose a reason for hiding this comment

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

Without this change the native gesture is broken, as it tries to work on a null view and gets errors. restoreViewStyles only changes view, thus there is not point in calling it then. When handler gets attached the view gets set and the init function called, thus restoreViewStyles is then called properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants