-
Notifications
You must be signed in to change notification settings - Fork 31
Integrating autologging surface tree to React native #264
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: main
Are you sure you want to change the base?
Integrating autologging surface tree to React native #264
Conversation
5c168f6
to
ed24ded
Compare
children | ||
); | ||
} | ||
// DOM-specific wrapper creation - commented out for React Native compatibility |
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.
With these types of things commented out, does the functionality you were testing in RN still work?
It seems like a lot of changes are specific to element reference, nodeRef is optional so it should be fine to just not pass for RN case.
The element specific things I'm wondering how we should approach conditionally doing vs. not doing those things depending on run context.
cc: @reshadi for thoughts
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, looks like all the changes is just about the element, we can make that optional, also in typescript we can use "interface merging" to define an HTMLElement type that can help us continue compiling code in RN even if those types are not defined.
I'm also thinking about splitting these components further to to server the the web/RN versions that have different useLayoutEffect.
} | ||
|
||
SurfaceProxy.init({ ...options, surfaceComponent: Surface }); | ||
// SurfaceProxy.init({ ...options, surfaceComponent: Surface }); |
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 confirmed there is no createPortal in RN right? This would be something else specific to web we need to conditionally do.
} | ||
// DOM-specific lookup - commented out for React Native compatibility | ||
// React Native doesn't have document.querySelectorAll | ||
// const el = document.querySelectorAll(`[${this.domAttributeName}="${this.domAttributeValue}"]`); |
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.
This seems like just a fallback, I wonder if it is needed if we fully trust ALSurfaceData to be the SoT
ALSurfaceUtils.ts
file, that extracts all platform agnostic logic and can be shared with web and react native