-
Notifications
You must be signed in to change notification settings - Fork 581
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
feat: send additional Sift events on Android #8991
Conversation
@MounirDhahri mind pulling this down and confirming you're still seeing events correctly fired? Would be greatly appreciated 🙂 |
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.
Nice! I tested this again and it's working well. I left a minor comment then I believe we can merge this
Screen.Recording.2023-07-14.at.10.56.58.mov
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.
Sorry, I missed something before. I added it now
SiftReactNative.setPageName(`screen_${initialRouteName}`) | ||
SiftReactNative.upload() | ||
} | ||
}, []) |
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.
issue(blocker): I think you need to trigger this only after the navigation is ready, otherwise it will be empty initially in case the navigation is not yet ready.
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.
Actually, even better, you might want to move this to be within onReady
prop in the NavigationContainer
to avoid watching the onReady
global state
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.
Good catch, thank you! Updated in 3d08ba5
Description
Followup to #8370.
Sift's React Native package doesn't correctly upload events without manual calls. This means we've had a few Sift events on Android (on app open + user login), but not as many as we'd like. The recommendation from the folks who worked on it is to add these calls on navigate.
PR Checklist
To the reviewers 👀
Changelog updates
Changelog updates
Dev changes
Need help with something? Have a look at our docs, or get in touch with us.