-
Notifications
You must be signed in to change notification settings - Fork 0
Add screen_view
and screen_leave
analytic events
#116
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
Conversation
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.
Pull Request Overview
This PR adds analytics tracking for screen view and leave events to the app. It enhances the Event class to support properties and implements automatic screen tracking through the Screen composable.
- Added
properties
field to Event interface with JSON serialization support - Created DesignEvent sealed class with ScreenView and ScreenLeave events
- Modified Screen composable to automatically track screen view/leave events with LaunchedEffect and DisposableEffect
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
analytics/src/main/kotlin/com/gravatar/analytics/Event.kt | Added properties interface to Event for JSON serialization |
design/src/main/kotlin/com/gravatar/app/design/analytics/DesignEvent.kt | New sealed class defining screen tracking events with properties |
design/src/main/kotlin/com/gravatar/app/design/components/Screen.kt | Modified to accept screenName parameter and automatically track screen events |
analytics/src/main/kotlin/com/gravatar/analytics/tracks/TracksTracker.kt | Updated to include event properties and prefix in track calls |
design/build.gradle.kts | Added analytics module dependency and Koin libraries |
analytics/src/test/kotlin/com/gravatar/analytics/tracks/TrackTrackerTest.kt | Updated test assertions to match new TracksTracker behavior |
loginUi/src/main/kotlin/com/gravatar/app/loginUi/presentation/login/LoginScreen.kt | Added screenName parameter to Screen call |
homeUi/src/main/kotlin/com/gravatar/app/homeUi/presentation/home/share/ShareScreen.kt | Added screenName parameter to Screen call |
homeUi/src/main/kotlin/com/gravatar/app/homeUi/presentation/home/profile/ProfileScreen.kt | Added screenName parameter to Screen call |
homeUi/src/main/kotlin/com/gravatar/app/homeUi/presentation/home/gravatar/GravatarScreen.kt | Added screenName parameter to Screen call |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
||
@Composable | ||
fun Screen( | ||
screenName: String, |
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.
Adding a required screenName
parameter is a breaking change to the Screen composable API. Consider making this parameter optional with a default value to maintain backward compatibility, or provide an overloaded function that doesn't require the screenName parameter.
screenName: String, | |
screenName: String = "", |
Copilot uses AI. Check for mistakes.
@@ -6,14 +6,20 @@ import com.gravatar.analytics.Tracker | |||
import java.util.UUID | |||
|
|||
internal class TracksTracker(private val tracksClient: TracksClient) : Tracker() { | |||
|
|||
internal companion object { | |||
const val TRACKS_EVENT_NAME_PREFIX = "gravatarandroid_" |
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.
❓ Is that a Track's requirement to prefix all the events?y
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.
As far as I remember, yes. In fact, we should use that concrete prefix as it was added to the allow list.
design/src/main/kotlin/com/gravatar/app/design/components/Screen.kt
Outdated
Show resolved
Hide resolved
b84ad4d
to
101c74a
Compare
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.
Description
This PR adds the properties to the Event class in the analytics module. Also, to verify that it works, I'm adding the
screen_view
andscreen_leave
events on theScreen
composable.This way, we can also work in parallel to add analytics, avoiding some conflicts.
Testing Steps
tracks
(you can use the filtergravatarandroid%
)flush
after each event and analyzing the network requests.