-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
feat: add google analytics gtag.js plugin #1702
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/docsify-core/docsify-preview/42cj1nPMt4MNgraowyET4t8gHDEe |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 6a3bd84:
|
Nice, thanks! Can you add a similar doc section as GA? Also it needs lint fix. |
@trusktr Hi, I update this merge request again, pls check it when you are free, thanks ~ |
@trusktr Hi, there is an error, |
Hey @ekoz. Updated and re-running the checks. |
@jhildenbiddle thanks, I read the docs https://playwright.dev/docs and have a try, run with How to test |
@jhildenbiddle Ping! 😄 |
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.
Hey @ekoz.
Apologies for the delayed response. Let's see if we can wrap this up and get it shipped. 😄
- Now that I better understand the purpose of the
gtag
snippet, I think we are better off creating a newgtag
plugin (as you originally proposed). I initially thought the snippet GA4-specific, but my understanding is that it is more of a universal snippet for Google services. Knowing this, it would be inappropriate to imply that this is a "Google Analytics" plugin ("ga.js") and instead make it clear that this is for Google's universal gtag plugin ("gtag.js"). - If we create a new "gtag" plugin, we'll need to update the
$docsify
property names as well (e.g.,gtag
instead ofga
). - I don't think we need to worry about the more complex scenarios handled by
gaCollector
as proposed in previous comments. Advanced users that require these configurations can simply callgtag()
in a<script>
on their own (see below). We should call this out in the docs as well.
What this all boils down to:
- Rename plugin to
gtag.js
and make necessary updates to accommodate name change - Fix issues outlined below
Thanks for your patience with all of this back and forth, @ekoz. As I said early on, I'm not a Google Analytics expert so much of the churn has been caused by me learning more about GA and the gtag
snippet as we go.
src/plugins/ga.js
Outdated
window.ga('send', 'pageview'); | ||
// usage: https://developers.google.com/analytics/devguides/collection/gtagjs/pages | ||
window.gtag('event', 'page_view', { | ||
page_title: document.title, |
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.
Please disable eslint's "camelcase" rules for under_score properties to prevent lint warnings:
window.gtag('event', 'page_view', {
/* eslint-disable camelcase */
page_title: document.title,
page_location: location.href,
page_path: location.pathname,
/* eslint-enable camelcase */
});
You can verify the fix by running npm run lint
.
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.
Hi, there is some questions:
- Do we still need to keep
ga.js
? docs/plugins.md
is necessary to keep the instructions forga
and add the instructions forgtag
?
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.
I believe we need to keep ga.js
(unmodified) because people are actively using this plugin. Your work would then be a new plugin, gtag.js
.
We should update the documentation and specify that the
We should update the documentation to specify that the ga.js
plugin is for "Google Universal Analytics". We can also provide a slightly modified warning that align with the one Google shows when you log into https://analytics.google.com/ and view a UA property:
Perhaps our message under the ga.js
heading can read:
Google's Universal Analytics service will no longer process new data in standard properties beginning July 1, 2023. Prepare now by setting up and switching over to a Google Analytics 4 property and docsify's
gtag.js
plugin.
How does that sound?
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.
Great~
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.
I just pushed the code, there is an exception in ci/codesandbox. I'm not sure what the reason is, please check it for me.
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.
I took a look and tried a few things in #1851 but I was unable to find the issue. This happened once before with Codesandbox it is it kinda of a PITA to troublehoot because the failures don't happen locally or in CI tests.
As much as I'd love to wrap this up, I don't have the spare cycles to devote to it right now. I will circle back as soon as I can devote some time to it.
In the meantime, thanks for being patient with this, @ekoz. Very much appreciated. 😄
src/plugins/ga.js
Outdated
function initGlobalSiteTag(id) { | ||
appendScript(id); | ||
|
||
window.dataLayer = window.dataLayer || []; |
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.
Let's move the window.dataLayer
and window.gtag
setup outside of the initGlobalSiteTag
function. This will allow users to call gtag()
so long as it is done after the plugin's <script>
tag:
Plugin:
window.dataLayer = window.dataLayer || [];
window.gtag =
window.gtag ||
function () {
window.dataLayer.push(arguments);
};
$docsify.plugins = [].concat(install, $docsify.plugins);
HTML:
<script src="//cdn.jsdelivr.net/npm/docsify@4/lib/docsify.min.js"></script>
<script src="//cdn.jsdelivr.net/npm/docsify@4/lib/plugins/gtag.min.js"></script>
<script>
// Advanced users can call `gtag()` on their own
window.gtag('event', 'conversion', {
allow_custom_scripts: true,
send_to: 'DC-ZZZZZZ/actions/locat304+standard',
});
</script>
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.
This is not work, I think we can ignore this function for the time being, and then pass the custom collector in through parameters when we need it later.
test/e2e/ga.test.js
Outdated
const reg = | ||
/googleads\.g\.doubleclick\.net|www\.google-analytics\.com|www\.googletagmanager\.com/g; | ||
if (request.url().match(reg)) { | ||
console.log(request.url(), response.status()); |
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.
Let's remove or comment out the console.log
tests so we don't see them in every test run.
can we merge this one? |
Summary
Google Universal Analytics (gtag.js) plugin, usage is the same as
src/plugins/ga.js
What kind of change does this PR introduce?
Feature
For any code change,
Does this PR introduce a breaking change? (check one)
If yes, please describe the impact and migration path for existing applications:
Related issue, if any:
#1695
Tested in the following browsers: