-
Notifications
You must be signed in to change notification settings - Fork 116
Track downloads in fathom #461
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
Conversation
|
Hi @runemadsen and thanks for this! I tried all events and everything seems to be working ✨ Note: one event doesn't appear on the screenshot below because of pagination.
Once @fdoflorenzano gives this PR the thumbs up on the code side, I'll happily merge and release. |
|
I had a closer look at the code, and it looks good to me! @fdoflorenzano do let me know if you notice anything that needs changing. In the meantime I'll go ahead with the merge. Thanks again for your help on this @runemadsen. |
fdoflorenzano
left a comment
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.
Looks great and works locally for me too! Just left a little question as a comment.
| trackWindows(0); | ||
| } else if (asset.os === 'macOS' && asset.bit === 'Intel 64-bit') { | ||
| trackMacIntel(0); | ||
| } else if (asset.os === 'macOS' && asset.bit === 'Apple Silicon') { | ||
| trackMacSilicon(0); | ||
| } else if (asset.os === 'Linux') { | ||
| trackLinux(0); | ||
| } else if (asset.os === 'Raspberry Pi' && asset.bit === '32-bit') { | ||
| trackPi32(0); | ||
| } else if (asset.os === 'Raspberry Pi' && asset.bit === '64-bit') { | ||
| trackPi64(0); |
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.
From what I understood from the docs, the callback don't need to have an argument defined (the default is 0 actually). Tested removing it locally and it works fine. Is there a reason behind actually setting the value that I'm not seeing @runemadsen? If so, we could add it as a comment then.
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.
Ah, good catch. I just saw them passing 0 in the documentation and assumed it worked like the official API. We can change that.
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.
Thanks @fdoflorenzano! Feel free to make another PR for this tweak.

This PR fixes #460 by tracking download events in Fathom Analytics. I implemented this with the
useGoalthat is a part of thegatsby-plugin-fathompackage. It's a little annoying that we need to run a hook per goal, but the library handles a lot of stuff around dev/prod and SSR so I'd rather rely on that.Adding @fdoflorenzano so they can take a look.
CC @SableRaf