Skip to content
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

Merged
merged 5 commits into from
Jul 17, 2023
Merged

feat: send additional Sift events on Android #8991

merged 5 commits into from
Jul 17, 2023

Conversation

mdole
Copy link
Contributor

@mdole mdole commented Jul 13, 2023

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

  • I have tested my changes on iOS and Android.
  • I hid my changes behind a feature flag, or they don't need one.
  • I have included screenshots or videos, or I have not changed the UI.
  • I have added tests, or my changes don't require any.
  • I added an app state migration, or my changes do not require one.
  • I have documented any follow-up work that this PR will require, or it does not require any.
  • I have added a changelog entry below, or my changes do not require one.

To the reviewers 👀

  • I would like at least one of the reviewers to run this PR on the simulator or device.
Changelog updates

Changelog updates

Dev changes

  • Adds additional Sift event calls on Android - mdole

Need help with something? Have a look at our docs, or get in touch with us.

@mdole
Copy link
Contributor Author

mdole commented Jul 13, 2023

@MounirDhahri mind pulling this down and confirming you're still seeing events correctly fired? Would be greatly appreciated 🙂

Copy link
Member

@MounirDhahri MounirDhahri left a 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

src/app/system/navigation/ModalStack.tsx Outdated Show resolved Hide resolved
src/app/system/navigation/ModalStack.tsx Outdated Show resolved Hide resolved
src/app/store/config/features.ts Show resolved Hide resolved
Copy link
Member

@MounirDhahri MounirDhahri left a 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()
}
}, [])
Copy link
Member

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.

Copy link
Member

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

Copy link
Contributor Author

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

@mdole mdole merged commit 7ea2e97 into main Jul 17, 2023
@mdole mdole deleted the mdole/sift-android branch July 17, 2023 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants