-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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(analytics): add 'logScreenView' API and deprecate setCurrentScreen
API.
#4145
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/invertase/react-native-firebase/46mcdvn95 |
setCurrentScreen
setCurrentScreen
setCurrentScreen
API.
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.
all documentation comments :-)
Co-authored-by: Mike Hardy <github@mikehardy.net>
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.
saw one little comment nit to pick :-)
Co-authored-by: Mike Diarmid <mike.diarmid@gmail.com> Co-authored-by: Mike Hardy <github@mikehardy.net>
Still WIP or good-to-go @russellwheatley :-) ? I could give it a final review pass and if it looks good maybe get it out... |
It should pass I think. Android tests flaked out last time so fingers crossed this time 🤞 |
Flaked again, on one I've seen - tracking flake cause here and rerunning #4058 (comment) |
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.
LGTM - will merge this as soon as E2E cooperates, or...I guess I'll have lots of fresh inspiration for de-flaking E2E ;-)
Hey I just see the new docs and for React Navigation, we should use just cc @mikehardy |
@MohamadKh75 I'm not sure screen class is that useful. But it is a valid parameter. How did it work when you tried it? (what did it look like in analytics, in other words) |
Well, I just upgraded to the newest version and use both |
I think it will just work. I don't believe there will be "novel" information. It should be the same screen tracking events, just from a different API call. Honestly, it is supposed to be an exceptionally boring change, just forward-porting to new underlying API calls. |
So, should I create a PR to add that to React Navigation? |
That is up to you? If React Navigation has any sort of documentation that cross-links with react-native-firebase here, and it needs an update, then sure. It's a different / unrelated repo so nothing I say is really authoritative, right? |
I mean in here 😁 |
@MohamadKh75 Sure! DIdn't realize it was actually our example :-) |
WIP. I'll request a review once I have a bit more feedback.
Description
setCurrentScreen
API. iOS & AndroidlogScreenView
Related issues
Release Summary
Checklist
Android
iOS
e2e
tests added or updated inpackages/\*\*/e2e
jest
tests added or updated inpackages/\*\*/__tests__
Test Plan
Think
react-native-firebase
is great? Please consider supporting the project with any of the below:React Native Firebase
andInvertase
on Twitter