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(analytics): add 'logScreenView' API and deprecate setCurrentScreen API. #4145

Merged
merged 5 commits into from
Aug 28, 2020

Conversation

russellwheatley
Copy link
Member

@russellwheatley russellwheatley commented Aug 26, 2020

WIP. I'll request a review once I have a bit more feedback.

Description

  • deprecated setCurrentScreen API. iOS & Android
  • added logScreenView

Related issues

Release Summary

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
  • My change includes tests;
    • e2e tests added or updated in packages/\*\*/e2e
    • jest tests added or updated in packages/\*\*/__tests__
  • I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes
    • No

Test Plan


Think react-native-firebase is great? Please consider supporting the project with any of the below:

@vercel
Copy link

vercel bot commented Aug 26, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/46mcdvn95
✅ Preview: https://react-native-firebase-git-russell-analytics-update.invertase.vercel.app

@russellwheatley russellwheatley changed the title feat(analytcis): add 'logScreenView'. dep. setCurrentScreen feat(analytics): add 'logScreenView' and deprecate setCurrentScreen Aug 26, 2020
@russellwheatley russellwheatley changed the title feat(analytics): add 'logScreenView' and deprecate setCurrentScreen feat(analytics): add 'logScreenView' API and deprecate setCurrentScreen API. Aug 26, 2020
Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

all documentation comments :-)

packages/analytics/lib/index.d.ts Show resolved Hide resolved
packages/analytics/lib/index.d.ts Show resolved Hide resolved
packages/analytics/lib/index.d.ts Show resolved Hide resolved
packages/analytics/lib/index.d.ts Outdated Show resolved Hide resolved
Co-authored-by: Mike Hardy <github@mikehardy.net>
Copy link
Collaborator

@mikehardy mikehardy left a 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 :-)

packages/analytics/lib/index.d.ts Outdated Show resolved Hide resolved
packages/analytics/lib/index.d.ts Outdated Show resolved Hide resolved
Co-authored-by: Mike Diarmid <mike.diarmid@gmail.com>
Co-authored-by: Mike Hardy <github@mikehardy.net>
@vercel vercel bot temporarily deployed to Preview August 28, 2020 15:26 Inactive
@mikehardy
Copy link
Collaborator

Still WIP or good-to-go @russellwheatley :-) ? I could give it a final review pass and if it looks good maybe get it out...

@russellwheatley
Copy link
Member Author

It should pass I think. Android tests flaked out last time so fingers crossed this time 🤞

@mikehardy mikehardy marked this pull request as ready for review August 28, 2020 16:56
@mikehardy mikehardy added Workflow: Needs Review Pending feedback or review from a maintainer. Workflow: Pending Merge Waiting on CI or similar labels Aug 28, 2020
@mikehardy
Copy link
Collaborator

Flaked again, on one I've seen - tracking flake cause here and rerunning #4058 (comment)

Copy link
Collaborator

@mikehardy mikehardy left a 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 ;-)

@mikehardy mikehardy removed the Workflow: Needs Review Pending feedback or review from a maintainer. label Aug 28, 2020
@mikehardy mikehardy merged commit 81c4e3b into master Aug 28, 2020
@mikehardy mikehardy deleted the @russell/analytics-update branch August 28, 2020 20:40
@mikehardy mikehardy removed the Workflow: Pending Merge Waiting on CI or similar label Aug 28, 2020
@MoKhajavi75
Copy link
Contributor

MoKhajavi75 commented Aug 29, 2020

Hey
Thanks for your efforts!

I just see the new docs and for React Navigation, we should use just class_name but we use both of screen_name and screen_class for React Native Navigation!
That is the correct behavior or I should create a PR for that missing line 😁?

cc @mikehardy

@mikehardy
Copy link
Collaborator

@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)

@MoKhajavi75
Copy link
Contributor

MoKhajavi75 commented Aug 29, 2020

Well, I just upgraded to the newest version and use both screen_name and screen_class! I should wait some time to get the analytics report.
Maybe @Salakar has an idea!

@mikehardy
Copy link
Collaborator

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.

@MoKhajavi75
Copy link
Contributor

So, should I create a PR to add that to React Navigation?

@mikehardy
Copy link
Collaborator

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?

@MoKhajavi75
Copy link
Contributor

I mean in here 😁

@mikehardy
Copy link
Collaborator

@MohamadKh75 Sure! DIdn't realize it was actually our example :-)

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.

4 participants