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

Slightly simplify JS construction in WC_Google_Gtag_JS #317

Closed
wants to merge 4 commits into from

Conversation

tomalec
Copy link
Member

@tomalec tomalec commented Oct 10, 2023

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.

Checks:

  • Does your code follow the WordPress coding standards?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully run tests with your changes locally?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Screenshots:

Detailed test instructions:

  1. Review the code generated from listing_click and event_tracking_code - it should be the same (ignoring whitespace)
  2. Review the code generated from load_analytics. It should be effectively equivalent

Additional details:

Changelog entry

@tomalec tomalec self-assigned this Oct 10, 2023
@tomalec tomalec requested a review from martynmjones October 10, 2023 17:54
@github-actions github-actions bot added the changelog: dev Developer-facing only change. label Oct 10, 2023
@tomalec tomalec added changelog: none Skip changelog entry for this PR and removed changelog: dev Developer-facing only change. labels Oct 10, 2023
' . 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);}
Copy link
Member Author

Choose a reason for hiding this comment

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

  1. I'm not sure if we or Google are bothered about arguments.callee.name that's pushed to window.dataLayer. If we don't mind pushing always gtag regardless of the custom name in tracker_var, we could simplify it to just `window.
Suggested change
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);}
Copy link
Member Author

Choose a reason for hiding this comment

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

  1. 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
Suggested change
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

@tomalec
Copy link
Member Author

tomalec commented Oct 14, 2023

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.
An maybe it will reduce merge conflicts if UA removal branch is going to take some time.

Copy link
Contributor

@martynmjones martynmjones left a 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?

@tomalec
Copy link
Member Author

tomalec commented Oct 24, 2023

I totally agree :)
I just had these bits as I was trying to understand the code, so I thought It might be a useful first step in the process of refactoring, or we could push it in the meantime before UA removal if that's going to take more time.
As well, I'm happy to discard this PR completely in favor of deeper refactoring.

@martynmjones
Copy link
Contributor

I just had these bits as I was trying to understand the code, so I thought It might be a useful first step in the process of refactoring, or we could push it in the meantime before UA removal if that's going to take more time.
As well, I'm happy to discard this PR completely in favor of deeper refactoring.

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 trunk and releasing as the extension does work at the moment it's just not particularly clean. That being said, if you disagree then let me know and I'll happily run further tests and review properly so that it can be merged.

@tomalec
Copy link
Member Author

tomalec commented Feb 26, 2024

closing in favor of #328

@tomalec tomalec closed this Feb 26, 2024
@eason9487 eason9487 deleted the dev/simplify-gtag branch February 27, 2024 02:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: none Skip changelog entry for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants