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

fix(analytics): capture engagement events on children of <a>'s #73

Merged
merged 4 commits into from
Nov 6, 2023

Conversation

jpezninjo
Copy link

@jpezninjo jpezninjo commented Oct 26, 2023

Goal: clicking on any part of a link should trigger engagement events.

Currently, our code is not setup to handle clicks on children of an anchor <a> tag.

This PR is a follow up improvement to #67 (comment)

To Do:

  • Copy button code to anchor code
  • Break out into re-usable function between button click and link click functions
  • Change button click function to only capture buttons

Image showing the bounding box of a nested <span> in an <a>

@jpezninjo jpezninjo requested a review from a team as a code owner October 26, 2023 20:52
@jpezninjo jpezninjo changed the title (DRAFT) fix(analytics): capture engagement events on children of <a>'s fix(analytics): capture engagement events on children of <a>'s Oct 27, 2023

linkClick.record({ target_url: closestLink.getAttribute('href') || '' })

recordEngagement(closestLink)
Copy link
Author

@jpezninjo jpezninjo Oct 27, 2023

Choose a reason for hiding this comment

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

capturing of links engagement happens here

function handleButtonClick(ev: MouseEvent) {
const eventTarget = ev?.target as Element
const closestButton = eventTarget.closest('button')

if (!closestButton)
Copy link
Author

Choose a reason for hiding this comment

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

This guard is a functional change. Currently, handleButtonClick captures engagement events on any element. Adding this guard is needed to prevent duplicate events from both here and handleLinkClick for clicks on links.

cc @wtfluckey

Choose a reason for hiding this comment

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

love it

@@ -33,24 +33,41 @@ export default defineNuxtPlugin((nuxtApp) => {
}
}

function recordEngagement(element: Element) {
Copy link
Author

Choose a reason for hiding this comment

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

broke out the code in handleButtonClick to this re-usable function

Copy link
Author

Choose a reason for hiding this comment

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

used early returns instead of if-statements and optional chaining

Copy link

@wtfluckey wtfluckey left a comment

Choose a reason for hiding this comment

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

makes sense to me!

@jpezninjo jpezninjo merged commit 7500fc2 into main Nov 6, 2023
3 checks passed
@jpezninjo jpezninjo deleted the fix/analytics-capture-nested-clicks branch November 6, 2023 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants