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

Switch to universal analytics #3495

Merged
merged 3 commits into from Jan 25, 2018

Conversation

Projects
None yet
3 participants
@davidfischer
Contributor

davidfischer commented Jan 9, 2018

This PR is based on #3085. We only need to merge one of them.

Therefore it contains:

  • Switches from legacy ga.js to new analytics.js, solving #1998
  • Switch to Universal Analytics must be done in readthedocs-sphinx-ext at the same time, see rtfd/readthedocs-sphinx-ext#29
  • Readthedocs pageviews and events are now all prefixed with rtfd tracker name, since setting the trackingId on a tracker is only allowed on creation (i.e. you can no longer call _setAccount in sphinx.js and sponsorship.js)
  • Google Analytics script element has been moved from the bottom of the page to top of <head> as per Googles instructions
  • Not using the Alternative async tracking snippet which should be a bit faster but "can degrade to synchronous loading and execution on IE 9 and older mobile browsers that do not recognise the async script attribute"
  • Needs to be tested by someone with access to RTFD Analytics. I've ported the events by referring to the Analytics API docs, but I don't know if they will appear correctly on an Analytics dashboard without changes.

On top of that PR, it also makes sure that pageviews and event tracking work on docs built before this change (using legacy analytics) as well as docs built after this change (using universal analytics).

VERIFICATION
I verified that flyout clicks and pageviews were tracked appropriately using the GA debugger extension on both previously built docs as well as docs containing the changes.

screen shot 2018-01-09 at 11 39 55 am

jessetan and others added some commits Sep 6, 2017

@davidfischer

This comment has been minimized.

Contributor

davidfischer commented Jan 9, 2018

I should mention that the bulk of this work was done in the previous PR by @jessetan. My changes were minor.

@ericholscher

Looks good to me.

@jessetan

This comment has been minimized.

Contributor

jessetan commented Jan 10, 2018

Is this still preferred over moving the (user) analytics stuff to the theme as suggested in rtfd/sphinx_rtd_theme#411 (comment)?

@davidfischer

This comment has been minimized.

Contributor

davidfischer commented Jan 10, 2018

Thanks for pointing me to that PR. I wasn't aware of the thought process of removing the analytics stuff from extensions (readthedocs-sphinx-ext). While I think putting analytics in the themes themselves is fine especially for people using the themes without Read the Docs, for RTD's purposes I think we still need some additions to track some things that regular theme users don't care about (eg. ads, version selector [flyout]).

@davidfischer

This comment has been minimized.

Contributor

davidfischer commented Jan 10, 2018

I think ideally that themes handle their own analytics however they want to do it. RTD analytics, however, should probably be handled by a script file loaded from media.readthedocs.org.

@jessetan

This comment has been minimized.

Contributor

jessetan commented Jan 12, 2018

Alright, so after this PR gets merged, we can proceed with adding user analytics stuff to the theme and reverting the changes to readthedocs-sphinx-ext. The RTD code will then only deal with GA specific to RTD, not page views for the users GA.

@davidfischer

This comment has been minimized.

Contributor

davidfischer commented Jan 12, 2018

That's my plan. Do you think that'll work for you?

@ericholscher ericholscher merged commit 6ab5626 into rtfd:master Jan 25, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment