-
Notifications
You must be signed in to change notification settings - Fork 88
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
[WNMGDS-2725] Add optional analytics tracking to ThirdPartyExternalLink
#3059
Conversation
(as far as I understand the spec)
This reverts commit adb7113.
"link_url": "foo", | ||
"parent_component_heading": " ", | ||
"parent_component_type": " ", | ||
"text": "External site linkThis link goes to an external site", |
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 noticed the text gets smooshed together here. Is there a chance this data would have an issue?
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.
Oh, so it's reading the SVG title + whatever the link label is. Instead of textContent
, would innerText
be a better option? It excludes the info from the SVG, but maybe that's OK?
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.
Good catch. I'll play around with some of those values
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.
Oh, I forgot that this is part of the getAnalyticsContentFromRefs
function, which is used in a lot of other places. innerText
appears to be undefined in a lot of these unit tests too. I think it's actually fine to just leave it.
…ink` (#3059) * Implement analytics for third party external links (as far as I understand the spec) * Add remaining event properties * Add this analytics config to storybook and prevent extra button engagement event * FOR TESTING IN STORYBOOK * Revert "FOR TESTING IN STORYBOOK" This reverts commit adb7113. * Update doc snapshots * Don't start out with a deprecated prop
Summary
thirdPartyExternalLinkSendsAnalytics
config property that controls whether theThirdPartyExternalLink
sends analytics events by defaultThirdPartyExternalLink
How to test
Checklist
[WNMGDS-####] Title
or [NO-TICKET] if this is unticketed work.Type
(only one) label for this PR, if it is a breaking change, label should only beType: Breaking
Impacts
, multiple can be selected.