-
Notifications
You must be signed in to change notification settings - Fork 202
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
Update payload of collection search analytics events #3957
Conversation
…on information on collection searches
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.
LGTM!
switch (this.collectionParams.collection) { | ||
case "creator": { |
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.
It's been a while since I saw switch
-case
because the object access pattern {}[]
appears so much more often. Nice!
const { sendCustomEvent } = useAnalytics() | ||
const { $sendCustomEvent } = useContext() |
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.
👌
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 works really well. The additional types and store values make the code clean, too ✨
* on collection pages, added in the | ||
* "Additional Search Views" project. | ||
*/ | ||
type CollectionProperties = { |
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.
Nice!
frontend/src/types/analytics.ts
Outdated
@@ -8,6 +8,7 @@ import type { FilterCategory } from "~/constants/filters" | |||
import { ResultKind } from "~/types/result" | |||
|
|||
import { RequestKind } from "./fetch-state" | |||
import { Collection } from "./search" |
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 and the previous imports use relative path, while we usually use absolute. I'd prefer to use absolute paths everywhere (and add import type
if it's a 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.
Hmmm, it feels weird that ESLint didn't catch and report this. We should look into that, but separately from this PR.
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, nice catch! These were auto imports from vscode so I didn't even check them. I'll fix
Fixes
Fixes #3619 by @obulat
Description
This PR updates the payload of search-related analytics events to include collection information on collection searches. These collection-related values are
null
on other searches. It also adds the existingkind
custom property to theLOAD_MORE_RESULTS
andREACH_RESULT_END
events.One aspect of this PR I do not like is the accessing the search store inside of the image cell and audio result components. It does feel ideal to use a standardized approach to getting the current search strategy and the value (string of the name of the tag, source, or source/creator) from one centralized place, but I don't like accessing a global store in a component which is repeated several times on the page.
Testing Instructions
additional_search_views
on http://localhost:8443/preferencesREACH_RESULT_END
event matches this partial payload{ kind: "search", collectionType: null, collectionValue: null }
LOAD_MORE_RESULTS
event matches this partial payload{ kind: "search", collectionType: null, collectionValue: null }
.SELECT_SEARCH_RESULT
event matches this partial payload{ kind: "search", collectionType: null, collectionValue: null }
.REACH_RESULT_END
event matches this partial payload{ kind: "collection", collectionType: "creator", collectionValue: {source}/{creatorName} }
LOAD_MORE_RESULTS
event matches this partial payload{ kind: "collection", collectionType: "creator", collectionValue: {source}/{creatorName} }
.SELECT_SEARCH_RESULT
event matches this partial payload{ kind: "collection", collectionType: "creator", collectionValue: {source}/{creatorName} }
.Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin