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

Update from analytics.js to gtag.js #1559

Merged
merged 6 commits into from
Jun 10, 2021
Merged

Conversation

mlubin
Copy link
Contributor

@mlubin mlubin commented Apr 4, 2021

The current Google Analytics code appears to be deprecated (it was triggering reminder emails for me).
I followed the instructions at https://developers.google.com/analytics/devguides/collection/upgrade/analyticsjs#analyticsjs_2_gtagjs to update the script.

Note that I don't understand the purpose of {'page': location.pathname + location.search + location.hash} and if we should do something similar in the new script. (Despite working at Google, I have no special knowledge of the Analytics API.)

@mlubin
Copy link
Contributor Author

mlubin commented Apr 4, 2021

Ping @IanButterworth and @MichaelHatherly who show up in blame for the Analytics code. See also #1121

@IanButterworth
Copy link
Contributor

IanButterworth commented Apr 4, 2021

{'page': location.pathname + location.search + location.hash}

I believe I added that to help Analytics tie search results to landing page tag i.e. section on a page, to try and highlight areas for improvement in SEO

@mortenpi mortenpi added Format: HTML Related to the default HTML output Type: Enhancement labels Apr 5, 2021
@mortenpi mortenpi added this to the 0.27.0 milestone Apr 5, 2021
@mortenpi
Copy link
Member

mortenpi commented Apr 5, 2021

Should we try to keep the snippet though @IanButterworth?

@IanButterworth
Copy link
Contributor

I don't know how gtag works (this is the first I've heard of it) so try the above, perhaps?

Co-authored-by: Ian Butterworth <i.r.butterworth@gmail.com>
src/Writers/HTMLWriter.jl Outdated Show resolved Hide resolved
Co-authored-by: Ian Butterworth <i.r.butterworth@gmail.com>
@@ -891,13 +891,13 @@ end
analytics_script(tracking_id::AbstractString) =
isempty(tracking_id) ? Tag(Symbol("#RAW#"))("") : Tag(:script)(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tag(:script) tag may be redundant with <script>

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the delay. But yes, you're right, this would be redundant. In fact, you'd end up with nested <script> tags, which means that I don't think this would work at all at the moment.

Instead, I'd recommend explicitly constructing both of the <script> tags in Julia. It should be fine if analytics_script returns an array of Tags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the syntax for constructing tags in Julia documented? (e.g., how do you do <script async src="...">?)

Copy link
Member

@mortenpi mortenpi Apr 13, 2021

Choose a reason for hiding this comment

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

Yep: https://juliadocs.github.io/Documenter.jl/stable/lib/internals/dom/

Also, heaps of usage examples in HTMLWriter.jl.

@mortenpi
Copy link
Member

I took the liberty of fixing this up. Testing it locally, it seems to submit the correct page name, including the fragment -- thanks @mlubin and @IanButterworth for figuring that out!

@mortenpi mortenpi merged commit 9f54fe2 into JuliaDocs:master Jun 10, 2021
@mlubin
Copy link
Contributor Author

mlubin commented Jun 10, 2021

Thanks @mortenpi!

Arkoniak pushed a commit to Arkoniak/Documenter.jl that referenced this pull request Jun 21, 2021
Co-authored-by: Ian Butterworth <i.r.butterworth@gmail.com>
Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Format: HTML Related to the default HTML output Type: Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants