-
Notifications
You must be signed in to change notification settings - Fork 2
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: track custom analytics events #243
Conversation
…y users on the platform
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.
Thanks for the PR @OchiengPaul442
I can confirm that I'm also able to see the custom analytics events you've started tracking from within the google analytics dashboard so nice that workflow is going as planned.
See a few comments inline, mostly around just trying to increase the amount of info getting tracked in analytics (namely resource and video ids, so that we can have a better insight into the resources users are accessing most frequently)
@@ -1,6 +1,7 @@ | |||
import { Component, ElementRef, HostBinding, Input, OnDestroy } from '@angular/core'; | |||
import { Capacitor } from '@capacitor/core'; | |||
import { ScreenOrientation } from '@capacitor/screen-orientation'; | |||
import { AnalyticsService } from '@picsa/shared/services/core/analytics.service'; |
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.
nit(blocking) - I see the tests are failing and they refer to this import in the errors (not sure if you have seen).
This is due to a convention with nx monorepos, that you shouldn't use package name prefixes when referring to files that area actually in the same package. As the analytics service and video-player component both sit within the shared
package, you should change the import to be a relative one instead
import { AnalyticsService } from '../../services/core/analytics.service';
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.
Hey @chrismclarke, I noticed the build errors earlier and was working on resolving them. Your explanation has provided clarity on the issue, and I’m now in a better position to address it. Thanks
@@ -74,6 +75,8 @@ export class VideoPlayerComponent implements OnDestroy { | |||
if (Capacitor.isNativePlatform()) { | |||
this.initialised = false; | |||
} | |||
// Track video play event | |||
this.analyticsService.trackVideoPlay(this.playerId); |
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.
nit(non-blocking): currently the playerId is a randomly generated id, so it doesn't really hold any analytics value. It might be good to implement player IDs as suggested in #241 (but not the full storage solution) so that we can track the id of the resource when passed to the video player and for logging in analytics
This could either be included in this pr (would only be a few lines of code), or as part of a follow-up
@@ -71,6 +73,8 @@ export class ResourcesToolService extends PicsaAsyncService { | |||
} | |||
|
|||
public async openFileResource(uri: string, mimetype: string) { | |||
// track the resource open event | |||
this.analyticsService.trackResourceOpen(); |
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.
nit(blocking): Nice we can track the resource open event but would be even better if we could track the id of the resource being opened. So in order to achieve this we either need to pass an extra argument to the openFileResource
method (e.g. id
), or call the tracking function within the resource-item-file
component itself which does have the id.
I think technically the better solution would probably be passing the id to the service, as that keeps the component more focused on the UI and service on side-effects
public trackVideoPlay(videoId: string) { | ||
this.firebaseAnalytics.logEvent({ | ||
name: 'video_play', | ||
params: { video_id: videoId, app_version: APP_VERSION }, |
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.
nit(non-blocking): app_version
will automatically be tracked in various other places so no need to explictily send with each analytics request, but very nice to have the video_id getting passed.
public trackResourceOpen() { | ||
this.firebaseAnalytics.logEvent({ | ||
name: 'open_resource_file', | ||
params: { app_version: APP_VERSION }, |
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.
nit(non-blocking): same as above, although would also be nice to have the resource_id
tracked as part of the the payload sent to analytics so that we can track research questions such as which resources are accessed most frequently
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.
Will work on that.
Hey @OchiengPaul442 - I can see you've removed the |
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.
Thanks @OchiengPaul442 , thanks for adding the updates.
I found one issue, there were additional components using the video player that were not passing the resource id, which stopped the video player from working. I fixed the component and added a fallback (console warning + random id) in case other developers use the player without passing an id parameter
I also made a few minor housekeeping improvements, I've prefixed all custom events with picsa_
to make them easier to locate in the analytics dashboard and moved the analytics enable/disable to the service itself (instead of app component) just to make it more clear for developers
I've included the fix and updates in b55a77f
When testing locally I didn't see any analytics events getting tracked, but then I realised that was because I use an adblocker (ublock origin) which automatically blocked GA when running on localhost (so had to disable). I don't think there's anything really to do about this from the code, more just to be aware in case other devs have similar issue.
Oh, and a very minor thing to note, you might see in the udpates I've changed your comment from using single //
to block /** */
.
The only reason for this is when using block /** */
comments vscode intellisense will detect and provide a popup. This works for all variables, functions etc. So I typically use single comments inline in functions when I just want to explain what the code is doing or minor notes, and block comments for names of variables and function descriptions to provide more general info for developers.
See the difference in screenshot below
But otherwise very nice addition to have, thanks for adding
Description
This PR:
Discussion
This PR resolves issue #220 by implementing the necessary changes.
Preview
Screenshots / Videos
Resources Event
Video Playback Event