-
Notifications
You must be signed in to change notification settings - Fork 18
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
Analytics for create a new collection #554
Conversation
bae95f0
to
4ca88bf
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.
looks good, however, the plausible "goals" are getting a bit unwieldy. we should consider a naming convention for better tracking. proposed a revision to this goal here (but requires change in plausible, too)
@@ -54,6 +54,8 @@ export type Events = { | |||
url: string; | |||
categoryDimension: string; | |||
}; | |||
|
|||
'CTA: New Collection': unknown; |
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.
What does "unknown" signify here?
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.
this represents the type of the data payload for the event. for example, here's what it is for filters:
napari-hub/frontend/src/hooks/usePlausible.ts
Lines 21 to 25 in 3e714bd
Filter: { | |
field: string; | |
value: string; | |
checked: boolean; | |
}; |
since the only thing we need from this event is whether it was fired or not, we don't really need a payload type, so I passed in unknown
for now. check this out for more info on the unknown
type: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-0.html#new-unknown-top-type
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.
Ah makes sense! Thanks for the overview!
Closes #513