-
Notifications
You must be signed in to change notification settings - Fork 42
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 external link tracking to Fathom integration #534
base: main
Are you sure you want to change the base?
Conversation
|
Changes look good. But before I merge this, can you install and test this version to verify it works on your end too? https://app.gitbook.com/integrations/test-fathom-update If all is good, I can get this merged in, and deployed to the production version :) |
Hey @addisonschultz , the tracking it self works, but I do not get a event for external site clicks. So it does not work yet. |
Can I simply update this PR and it will reflect the changes in my space? |
You can update this PR - and I'll update the integration after! |
f337239
to
6255c24
Compare
Hi @addisonschultz, I made a mistake in the replace function call... Can you please update the integration? |
It should be updated now - you can install with the same link! https://app.gitbook.com/test-fathom-update |
@addisonschultz please update again. |
Done ✅ The integration looks good - is it working as expected on your end as well? |
Hi @addisonschultz, now it works as expected! |
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.
If you could make these changes, I'll be happy to merge and get this all live on the production version 😄
@@ -36,11 +36,27 @@ summary: | | |||
categories: | |||
- analytics | |||
configurations: | |||
space: |
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.
We don't use this space
configuration key anymore - so you can omit this one from the configuration (See main
right now, as it's been removed
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.
Done
integrations/fathom/src/index.ts
Outdated
return new Response(script.replace('<TO_REPLACE>', siteId), { | ||
const trackExternalLinks = | ||
environment.siteInstallation?.configuration?.track_external_links ?? | ||
environment.spaceInstallation?.configuration?.track_external_links ?? |
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.
You can omit environment.spaceInstallation...
from here as well
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.
Done
Sure, I will do this. Please keep this PR open - I want to provide some changes to the event information. window.fathom.trackEvent('External link clicked: ' + domainName); The |
@addisonschultz ok, please update again for a final test |
Sorry for the delay! It's just been updated again |
Looking into it 😕 |
@KevinRohn can you give it a try now? We should have fixed the bug causing this! |
This PR adds an option to track external link clicks in the Fathom integration. Regarding to the issue #450
When enabled, the integration tracks clicks on links leading to external domains. It sends an event to Fathom with the format "External link clicked: domain".
index.ts
to pass the new option to the scriptgitbook-manifest.yaml
with the new configurationfathomScript.raw.js
implement external link trackingsiteID
renamed tositeId
, this naming is also for other integrationsI hope I did anything correct, because I was not able to test it without the script injection scope.
Cheers,
Kevin