-
Notifications
You must be signed in to change notification settings - Fork 70
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
Slightly simplify JS construction in WC_Google_Gtag_JS #317
Conversation
Use `gtag` locally.
3dcc524
to
dbdeca6
Compare
' . self::tracker_var() . "('js', new Date()); | ||
// Publish gtag under custom name to the global scope. | ||
// Preserve the custom `arguments.callee.name`, for backward compatibility (not sure if even needed). | ||
const gtag = window.$gtag_name = function $gtag_name(){dataLayer.push(arguments);} |
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'm not sure if we or Google are bothered about
arguments.callee.name
that's pushed towindow.dataLayer
. If we don't mind pushing alwaysgtag
regardless of the custom name intracker_var
, we could simplify it to just `window.
const gtag = window.$gtag_name = function $gtag_name(){dataLayer.push(arguments);} | |
window." . $custom_gtag . " = function gtag(){dataLayer.push(arguments);} |
' . self::tracker_var() . "('js', new Date()); | ||
// Publish gtag under custom name to the global scope. | ||
// Preserve the custom `arguments.callee.name`, for backward compatibility (not sure if even needed). | ||
const gtag = window.$gtag_name = function $gtag_name(){dataLayer.push(arguments);} |
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.
- Or if the
tracker_var
was just to avoid conflicts with other plugins using the "gtag", and not to allow customization of something very specific. We can hide it in our own namespace/scope. Like
const gtag = window.$gtag_name = function $gtag_name(){dataLayer.push(arguments);} | |
window.wcgai.gtag = function gtag(){dataLayer.push(arguments);} |
And use it as wcgai.gtag( 'event', //...
everywhere else
Actually, I think we can also merge this change to the develop, as it is not related to UA removal, and should work as well there. |
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 @tomalec, thanks for your work to simplify the tracker.
I'm wondering if there's any benefit to us continuing to setup the tracker the way we are though? As we discussed on the call a couple of weeks back, we're going to remove as much inline JS as possible and instead trigger function calls with relevant data.
In class-wc-google-gtag-js.php
we could setup an array with all relevant settings and then trigger a call to a JS function that initializes the tracker function which would make load_analytics
a lot simpler and far more readable.
What do you think?
I totally agree :) |
Definitely useful and thanks for opening the PR! As we are going to be making some pretty significant changes I'd be in favor of jumping straight into a deeper refactoring of how it's working at the moment. While the project will take a while longer, I'm not sure I see the benefit in pushing to |
closing in favor of #328 |
Changes proposed in this Pull Request:
These are a few cosmetic changes I made trying to understand what code is being constructed by PHP. That should be fully backward compatible, but makes the code slightly more readable at least to me.
Use
gtag
locally.Checks:
Screenshots:
Detailed test instructions:
listing_click
andevent_tracking_code
- it should be the same (ignoring whitespace)load_analytics
. It should be effectively equivalentAdditional details:
Changelog entry