-
Notifications
You must be signed in to change notification settings - Fork 14
ksearch: upgrade to Events API #508
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
base: main
Are you sure you want to change the base?
Conversation
|
Current unit coverage is 90.83735203857957% |
src/models/SearchEventPayload.ts
Outdated
| searchTerm?: string; | ||
| } | ||
|
|
||
| //todo - how many of these do we want to delete? |
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.
I want to prune these down - open to what we use as our canonical list.
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 there a reason we want to prune these down? At the very least, I imagine we want things like map pin, auto complete selection, the paginates, chat impression, search clear, etc.
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.
So as we discussed, let's pare this down based on user event types and then consider any remaining enum types more individually. If it's clearly search related or has been used before, include it. If it's clearly for another product, exclude it, etc. Use your discretion, but feel free to loop in Baigel / others if you want a 2nd opinion while I'm out.
src/models/SearchEventPayload.ts
Outdated
| searchTerm?: string; | ||
| } | ||
|
|
||
| //todo - how many of these do we want to delete? |
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 there a reason we want to prune these down? At the very least, I imagine we want things like map pin, auto complete selection, the paginates, chat impression, search clear, etc.
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.
Generally looks good! Had some comments about specific details, but I like the wrapper, and overall the changes make sense.
src/models/SearchEventPayload.ts
Outdated
| searchTerm?: string; | ||
| } | ||
|
|
||
| //todo - how many of these do we want to delete? |
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.
So as we discussed, let's pare this down based on user event types and then consider any remaining enum types more individually. If it's clearly search related or has been used before, include it. If it's clearly for another product, exclude it, etc. Use your discretion, but feel free to loop in Baigel / others if you want a 2nd opinion while I'm out.
This PR upgrades our analytics calls to the next major version, also called the Events API. As part of this work, analytics calls have changed shape, and some deprecated properties have been dropped. J=WAT-4651 TEST=auto,manual Updated auto tests. Ran test site locally and saw events all the way to snowflake.
analytics import
39e70b3 to
d1705b5
Compare
This PR upgrades our analytics calls to the next major version, also called the Events API. As part of this work, analytics calls have changed shape, and some deprecated properties have been dropped.
J=WAT-4651
TEST=auto,manual
Updated auto tests. Ran test site locally and saw events all the way to snowflake.