Skip to content
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

Merged
merged 3 commits into from
Apr 5, 2024

Conversation

Mike-Heneghan
Copy link
Contributor

What

Why

  • Step towards clearer organisation and documentation of analyticsProvider
  • Should hopefully be a clearer pattern for adding tracking of new events

@Mike-Heneghan Mike-Heneghan self-assigned this Apr 4, 2024
@Mike-Heneghan Mike-Heneghan force-pushed the mh/consolidate-tracking-events branch from 14d36d4 to 042e2a3 Compare April 4, 2024 16:39
@Mike-Heneghan
Copy link
Contributor Author

@Mike-Heneghan Mike-Heneghan marked this pull request as ready for review April 4, 2024 17:05
@Mike-Heneghan Mike-Heneghan changed the base branch from mh/move-analytics-mutations-into-file to main April 4, 2024 17:06
- 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]>
@Mike-Heneghan Mike-Heneghan force-pushed the mh/consolidate-tracking-events branch from 042e2a3 to 5f30620 Compare April 4, 2024 17:08
Copy link

github-actions bot commented Apr 4, 2024

Removed vultr server and associated DNS entries

@Mike-Heneghan Mike-Heneghan requested a review from a team April 4, 2024 17:11
Copy link
Contributor

@DafyddLlyr DafyddLlyr left a 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(),
Copy link
Contributor

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Contributor Author

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* The flow direction when a new analytics session is create is
* The flow direction when a new analytics session is created is

Copy link
Contributor Author

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

@Mike-Heneghan Mike-Heneghan merged commit 2c53f38 into main Apr 5, 2024
12 checks passed
@Mike-Heneghan Mike-Heneghan deleted the mh/consolidate-tracking-events branch April 5, 2024 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants