-
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
refactor: create trackEvent function and apply updates in relevant files #2986
Conversation
14d36d4
to
042e2a3
Compare
- As per: #2597 (comment) - Consolidating individual tracking event functions into a single function with a case statement - Step towards clearer organisation and documentation of analyticsProvider Co-authored-by: Dafydd Llŷr Pearson <[email protected]>
042e2a3
to
5f30620
Compare
Removed vultr server and associated DNS entries |
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.
Really strong refactor - this seems like it would be much easier to maintain and expand.
A few typo/type suggestions but no showstoppers.
trackNextStepsLinkClick: () => Promise.resolve(), | ||
trackFlowDirectionChange: () => Promise.resolve(), | ||
trackBackwardsNavigation: () => Promise.resolve(), | ||
trackEvent: () => Promise.resolve(), |
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.
Much neater and cleaner 💪
async function trackHelpClick(metadata?: HelpClickMetadata) { | ||
if (shouldSkipTracking()) return; | ||
|
||
async function updateMetadata(mutation: any, metadata: Metadata) { |
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.
async function updateMetadata(mutation: any, metadata: Metadata) { | |
async function updateMetadata(mutation: DocumentNode, metadata: Metadata) { |
I think this is right - this should be the type of strings wrapped in gql
tags.
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 spotting this @DafyddLlyr, I did the classic "I'll come back to this later and totally forgot 😅" should be fixed here: a7ac3b1
}; | ||
|
||
/** | ||
* The flow direction when a new analytics session is create is |
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.
* The flow direction when a new analytics session is create is | |
* The flow direction when a new analytics session is created is |
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.
Should be fixed here: f811fe2
What
Why