-
Notifications
You must be signed in to change notification settings - Fork 4
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
Adds Google Analytics #41
Conversation
Coverage remained the same at 100.0% when pulling e2ce3a73648b7646aee43499d69ee298814ecc77 on matt/analytics into 79ce0da on master. |
Coverage remained the same at 100.0% when pulling 575b492bd390170cf366080d46de7145a7358dfe on matt/analytics into 79ce0da on master. |
Coverage remained the same at 100.0% when pulling 98dee4bd24e8d7261f45f611adbe7d4bc71e6a7d on matt/analytics into 79ce0da on master. |
Coverage remained the same at 100.0% when pulling 9734d82201eb0164a4d844f29911d9df5a48d434 on matt/analytics into 79ce0da on master. |
I need to think about how to not run against any GA during full stack tests. Or even in local development I often wouldn't want to send stuff to GA. It shouldn't be a huge change. Mostly I think we want to check for an GA_PROPERTY in ENV and only run this if so (and use that GA_PROPERTY value instead of storing property values in the code). To be able to read ENV you'll need to rename this to |
That makes sense. I'll rework this to check for an ENV variable and perform
this only if present. There will probably be times when we _do_ want to
send data to GA in testing, but I can see not wanting to do that in all
cases.
|
Coverage remained the same at 100.0% when pulling 48f061b15f1e007e2a8633fe4eb8e8c882b62504 on matt/analytics into 79ce0da on master. |
@matt-bernhardt Sorry for misleading information previously. I think this is best done in the application template and not in an external stylesheet due to the way the application pipeline only compiles at deploy time for production applications. This would work fine in dev environments, but deployed it will not work as intended. I.E. changing the ENV setting (or removing it) will have no effect until a redeploy when the pipeline recompiles. So... can you take your script and move it back to the |
Coverage remained the same at 100.0% when pulling e034d7aec53e34adf4a40c7a809cfb9131f88e49 on matt/analytics into 79ce0da on master. |
Coverage remained the same at 100.0% when pulling 934a0b1cf4056578f0b937653ea8c0653b60ef3c on matt/analytics into 79ce0da on master. |
I think this may be ready for squashing now? The presence/absence of |
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 good, thanks!
Yeah, squash and merge. I added / removed the key on Heroku and it worked as expected. I'll transfer to the shared account when I'm at stabler wifi. |
This is optional, and for now based only on pageviews.
e034d7a
to
0e36ed2
Compare
This takes the stock GA code, de-minimizes it to pass the eslint check on CodeClimate, and adds a ternary check of the domain name to swap between registering activity to the test or production account.
As a first step, I think the goal is just to track pages. A further goal would be to adopt event tracking in order to record the items returned from the various tools, or to record where the user moves from the result page.
A second needed change before this launches is probably to re-consider how we check for the app being in production. I'd welcome @JPrevost thoughts here.
(I'm also happy to squash this branch before merging - so far I've been leaving the commits separate)
When merged, this closes #26.