Allow Google Analytics to use gtag.js #601
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
😢 But, you provided enough detail that we can look at this PR anyway 😄 I am trying to think if this will cause any backward compatibility problems - don't think so. I added @yangshun to see if he thinks there are any issues here. |
|
Moving it into @ramiel the tests are failing, I think it's because the code has to be prettified first. Run Note that making this change will break users who are relying on IMO if we want to merge this, it is a breaking change and will require a major version bump. But I do think this upgrade will have to be done eventually. Alternatively, add an option into cc @JoelMarcey |
ramiel
force-pushed
the
ganalytics-on-head
branch
2 times, most recently
from
0d20196
to
869d02f
Compare
|
So, I rebased and run prettier, now tests pass. For the moment I'd leave this PR as is until you decide what's better. If you say to have an option in siteConfig, I'll create it. |
|
@ramiel @yangshun I am ok with adding a siteConfig option for this. Let's go that route. We can call it Let's do it! |
|
Ok, I'll do these changes soon |
The code now uses the new gtag script to track the website on google analytics. The migration is explained [here](https://developers.google.com/analytics/devguides/collection/gtagjs/migration). This will help to solve issues such as facebook#375. As indicated at https://support.google.com/analytics/answer/1008080 the analytics code must be placed in the head tag, so it is moved there.
ramiel
force-pushed
the
ganalytics-on-head
branch
from
ae436f5
to
7ff6168
Compare
| )} | ||
| {this.props.config.gaTrackingId && | ||
| this.props.config.gaGtag && ( | ||
| <script |
There was a problem hiding this comment.
Couldn't use combine the the two <script> into one gaTrackingID && gaGtag check, instead of having two of the same check?
There was a problem hiding this comment.
Basically
{this.props.config.gaTrackingId &&
this.props.config.gaGtag && (
<script
async
src={`https://www.googletagmanager.com/gtag/js?id=${
this.props.config.gaTrackingId
}`}
/>
<script
dangerouslySetInnerHTML={{
__html: `
window.dataLayer = window.dataLayer || [];
function gtag(){dataLayer.push(arguments); }
gtag('js', new Date());
gtag('config', '${this.props.config.gaTrackingId}');
`,
}}
/>
)}
|
In react 15 you cannot return two children without a common parent. This is why I did like that. But I can check again,maybe I'm wrong |
yangshun
changed the title
|
Any idea when this will make it to a release? |
|
@j-f1 I am going to try to get out a release this weekend. |
The code now uses the new gtag script to track the website on google analytics.
The migration is explained here.
This will help to solve issues such as #375.
As indicated at https://support.google.com/analytics/answer/1008080
the analytics code must be placed in the head tag, so it is moved there.
Motivation
I need to link my website to google search console so, I need the google analytics code to be setup properly
Have you read the Contributing Guidelines on pull requests?
No :)
Test Plan
The new code is on the now, instead of the body. There were no test for this before and I added none
Related PRs
None