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

fix(listen): adding in baseline listen analytics #598

Merged
merged 6 commits into from
Apr 18, 2023
Merged

Conversation

bassrock
Copy link
Member

@bassrock bassrock commented Apr 13, 2023

Summary

Adds in callbacks to listen, for listen analytics

Implementation Details

  • Guessed on most of them, we can add more detail in the future too with the MediaPlayer entity but we need to update listen to provide that info.

PR Checklist:

  • Added Unit / UI tests
  • Self Review (review, clean up, documentation, run tests)
  • Basic Self QA

Screenshots

@bassrock bassrock marked this pull request as ready for review April 14, 2023 22:47
@bassrock bassrock requested a review from a team as a code owner April 14, 2023 22:47
@bassrock bassrock requested review from cyndichin and CMasterson and removed request for a team April 14, 2023 22:47
}

func listenDidDismissPlayer(_ player: PKTListenAudibleQueuePresentationContext) {
}

func listenDidDismiss() {
tracker.track(event: Events.Listen.Closed())
Copy link
Member Author

Choose a reason for hiding this comment

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

You'll need to ask @nzeltzer why there are 2 call backs :)

@pocket-ci
Copy link
Contributor

pocket-ci commented Apr 14, 2023

Messages
📖 No SwiftLint violations! 🎉
📖 Project coverage: 78.57%
📖 Edited 2 files
📖 Created 0 files

Analytics: Coverage: 71.68

File Coverage
Listen.swift 4.52% ⚠️

PocketKit: Coverage: 78.08

File Coverage
Listen.swift 40.98% ⚠️

Generated by 🚫 Danger Swift against 9bdbd50

@bassrock bassrock enabled auto-merge (squash) April 14, 2023 23:07
Copy link
Contributor

@cyndichin cyndichin left a comment

Choose a reason for hiding this comment

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

will there be UI tests for these analytics? approving as code looks good!

@bassrock
Copy link
Member Author

@cyndichin yes once we get accesibility identifiers added to listen

@bassrock bassrock merged commit fa47624 into develop Apr 18, 2023
@bassrock bassrock deleted the listen-analytics branch April 18, 2023 01:32
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