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

Add Google Analytics Autolink Helper #1125

Merged
merged 33 commits into from
Apr 2, 2019
Merged

Conversation

sghoweri
Copy link
Contributor

Jira

http://vjira2:8080/browse/WWWD-2988

Summary

Adds a new Google analytics autolink helper package to make it easy to wire up Bolt components with Google Analytics Linker plugin so components with external domain URLs (regardless on how a component renders, what browser a component is rendering in, and whether or not a component renders / re-renders) are much easier to track.

Details

  • While no changes to any existing Bolt components was technically required to get this wired up (since this works by hooking into the rendered custom event every Bolt component emits when initially rendering + re-rendering when data changes), this PR:
  • Formalizes this integration point (ensure there isn't any unexpected API changes)
  • Makes sure this approach continues to work as expected
  • Adds testing coverage to verify a variety of different use cases / situations / configuration options
  • Provides overall docs on usage + configuration

How to test

  • Confirm the build + new tests added all pass on Travis
  • Read through the docs to make sure usage and configuration all make sense

Three things in particular @danielamorse I'd like to get your opinion on...

  1. Where exactly in the monorepo codebase do you think this package makes the most sense to live in? I settled on putting this in a new packages/analytics/autolink folder with the idea that additional analytics-specific packages will be added, however I also considered having this live in the components folder (despite this package only applying component behavior vs being a "traditional" component).

  2. Given that this isn't a "traditional" component, this also unfortunately means it doesn't have docs that automatically show up in the docs / Pattern Lab which I find to be problematic.

  3. Thoughts on this being packaged up as a super simple custom element? ex. <bolt-autolink>? That could solve the need to have a proper home for this in addition providing a declarative way to configure the domains being tracked (ex. allow domains to declaratively get passed along via the custom element vs only providing a global JS config option which imho, really doesn't feel like it scales...)

@sghoweri sghoweri added this to the v2.4.0 milestone Mar 22, 2019
…ers + rename autolink test configs to have a `.data.js` file extension used to exclude from Jest
@sghoweri sghoweri requested a review from danielamorse March 22, 2019 16:14
Copy link
Collaborator

@danielamorse danielamorse left a comment

Choose a reason for hiding this comment

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

Looking good! Some notes:

And to your questions:

  1. I like it here in /packages since it isn't a traditional component.
  2. Sounds like we need a separate category of component (tools, helpers, etc). Maybe our future docs can include more (or all) of these other packages, but in a separate bucket.
  3. So <bolt-autolink> would wrap a <bolt-link>? I could see that being useful. But I think the global JS config would be the primary way this gets used. So, let's keep that whatever we do. And I wouldn't necessarily prioritize the custom-element version just to surface docs. Something for the backlog?

Anyway, nice job! I'm approving. Should be good to merge once the build is passing and those typos fixed.

@@ -0,0 +1,93 @@
# Bolt Autolink

Helper library to automatically applies GA [autolink](https://developers.google.com/analytics/devguides/collection/analyticsjs/linker) click tracking query strings to Bolt components that point to external domains.
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo: "to automatically apply"

</script>
```

Option 2. This can also be configured via a Drupal config. For example:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice, great docs! Especially like the Drupal example.

@@ -0,0 +1,3 @@
$schema: 'http://json-schema.org/draft-04/schema#'
title: 'Bolt Autolink'
description: 'Automatically applies GA autolink click tracking to other Bolt components that containin URLs to external domains'
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo: "components that contain"

@@ -0,0 +1,107 @@
import URLSearchParams from '@ungap/url-search-params'; // URLSearchParams poly for older browsers
Copy link
Collaborator

Choose a reason for hiding this comment

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

I haven't seen this type of interaction between web component events and external scripts yet. Let's review in dev huddle.

@sghoweri sghoweri mentioned this pull request Apr 2, 2019
@sghoweri sghoweri merged commit fd98763 into master Apr 2, 2019
@sghoweri sghoweri deleted the feature/analytics-autolink branch April 2, 2019 19:38
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