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

FIX: added gtag date and moved google analytics to header #439

Merged
merged 10 commits into from
Sep 16, 2021

Conversation

jackminchin
Copy link
Contributor

Coming from the jupyter-book project. Having some issues with the gtag, it was getting inserted in the footer and not the header as Google documentation suggests.

Additionally, gtag('js', new Date()); was missing from the snippet.

Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

Seems good to me!

jackminchin and others added 2 commits August 3, 2021 12:04
Co-authored-by: Chris Holdgraf <choldgraf@gmail.com>
Co-authored-by: Chris Holdgraf <choldgraf@gmail.com>
@jorisvandenbossche jorisvandenbossche changed the title gtag moved to header (per gtag docs) and added gtag date FIX: added gtag date and moved google analytics to header Aug 9, 2021
@jorisvandenbossche
Copy link
Member

jorisvandenbossche commented Aug 9, 2021

The gtag fix looks good, thanks!
About moving it to the header, is that strictly needed to fix your problem? To be clear I am no expert, for this has worked for years (and eg readthedocs theme also puts it after the content at the end of the body)

@jackminchin
Copy link
Contributor Author

Hey @jorisvandenbossche,

It's a strange one, I was having issues with getting the gtag working, it was installed on the page and I could see that it was loading the script from google, but not sending any data.

I checked the gtag docs from google and they suggested that it should be in the header, but I also noticed that a line that seems to initialise the date in the gtag snippet was also missing.

I tried to just add the date tag first on a local machine, this didn't work and neither did just moving to the header, but making both changes made it work in my case - both in development and production.

It seems like, in my case, both fixes were necessary but not sufficient to get the gtag to send data. Strange that it has been working for others though.

@erogluorhan
Copy link

erogluorhan commented Aug 25, 2021

Thanks for this fix! Is there a plan to fix this PR's tests, get it merged and released soon?

@choldgraf
Copy link
Collaborator

Since google recommends putting the JS in the header I think it's fine for us to adopt that practice.

I am happy to merge this one once the tests are happy, unless @jorisvandenbossche objects in which case I'm happy to discuss further

@jackminchin
Copy link
Contributor Author

Hey @erogluorhan! No problem, looking to contribute some more meaningful stuff when I get a bit of time!

@scottyhq
Copy link

scottyhq commented Sep 1, 2021

Just wanted to link to the corresponding issue in the jupyterbook repo jupyter-book/jupyter-book#1300 . Would love for this fix to go in as it seems necessary for the new-style tracking ids.

@erogluorhan looks like you figured out how to fix currently failing tests in your linked PR?

@erogluorhan
Copy link

erogluorhan commented Sep 2, 2021

Just wanted to link to the corresponding issue in the jupyterbook repo executablebooks/jupyter-book#1300 . Would love for this fix to go in as it seems necessary for the new-style tracking ids.

@erogluorhan looks like you figured out how to fix currently failing tests in your linked PR?

Hi @scottyhq , yes I made a simple workaround, which worked for us. We are using sphinx_book_theme that extends pydata_sphinx_theme. I created a new layout that extends sphinx_book_theme\layout.html and also adds the Google Analytics snippet to extrahead. Additionally, I am suppressing the use of google_analytics_id in _config.yml for now since it triggers pydata_sphinx_theme to add the snippet to the footer (which is the issue this PR tries to address).

@choldgraf
Copy link
Collaborator

I think this one is just waiting for the tests to pass - it looks like some are related to the gtag change: https://github.com/pydata/pydata-sphinx-theme/runs/3279225091#step:8:162 I think that's because the test is selecting for the wrong parent element now

@jackminchin do you think you'll have a chance to give this a shot soon?

@jackminchin
Copy link
Contributor Author

@choldgraf, yes I can probably try and get this test running properly again later on today.

@choldgraf
Copy link
Collaborator

Friendly ping @jackminchin 🙂

@jackminchin
Copy link
Contributor Author

@choldgraf sorry for the delay, I have been looking into this today and the tests run fine on my local machine. Any idea why this is happening?

@scottyhq
Copy link

scottyhq commented Sep 13, 2021

@jackminchin looks like you've accidently added some .DS_Store files in your last commits, the failing google-analytics tests were:

=========================== short test summary info ============================
FAILED tests/test_build.py::test_new_google_analytics_id - TypeError: argumen...
199
FAILED tests/test_build.py::test_old_google_analytics_id - TypeError: argumen...
200
======================== 6 failed, 25 passed in 15.73s =========================

Old-style analytics ids insert HTML like this at the end of the body (view-source:https://jupyterbook.org/intro.html)

<script async="" src="https://www.google-analytics.com/analytics.js"></script>
<script>
                        window.ga = window.ga || function () {
                            (ga.q = ga.q || []).push(arguments) };
                        ga.l = +new Date;
                        ga('create', 'UA-52617120-7', 'auto');
                        ga('set', 'anonymizeIp', true);
                        ga('send', 'pageview');
                    </script>

Functional new-style html put the following in the header (view-source:https://snowex-hackweek.github.io/website/intro.html):

    <!-- Google Analytics - Global site tag -->
    <script async src="https://www.googletagmanager.com/gtag/js?id=G-QM84T4SB76"></script>
    <script>
        window.dataLayer = window.dataLayer || [];
        function gtag(){dataLayer.push(arguments);}
        gtag('js', new Date());

        gtag('config', 'G-QM84T4SB76');
    </script>

  </head>

@choldgraf or others not sure where index_html.select() in the existing tests is documented, but rather than relying on the positional index of the google script would it be ok to iterate on all of them and filter on the src label src.startswith(https://www.google-analytics.com/analytics.js) or src.startswith('https://www.googletagmanager.com/gtag/js?')

    def test_old_google_analytics_id(sphinx_build_factory):
        confoverrides = {"html_theme_options.google_analytics_id": "UA-XXXXX"}
        sphinx_build = sphinx_build_factory("base", confoverrides=confoverrides)
        sphinx_build.build()
        index_html = sphinx_build.html_tree("index.html")
        # This text makes the assumption that the google analytics will always be
        # the one before last script tag found in the document.
        script_tag = index_html.select("script")[-1]
    
>       assert "ga" in script_tag.string
E       TypeError: argument of type 'NoneType' is not iterable

@jorisvandenbossche
Copy link
Member

I pushed a few commits to update the PR: 1) merged latest master to get the test fixes for sphinx, 2) removed the unrelated changes (DS_Store files) and 3) fixed the tests (I did the quick fix with harcoding it to the "second-to-last-last script", but @scottyhq suggestion of using a more robust search might be better long term).

For the rest I am fine with the PR (as mentioned above, no expert on this, I was only wondering why the move to the header was needed since we are already using a long time in practice. But since google recommends it, and some users seem to need it, that sounds good)

@jorisvandenbossche
Copy link
Member

Tests are all passing, so going to merge this. Thanks @jackminchin !

@jorisvandenbossche jorisvandenbossche merged commit 055ae27 into pydata:master Sep 16, 2021
matthewevans pushed a commit to aptech/pydata-sphinx-theme that referenced this pull request Dec 16, 2021
Co-authored-by: Jack Minchin <jack@thisisstratum.co.uk>
Co-authored-by: Chris Holdgraf <choldgraf@gmail.com>
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
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.

5 participants