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

Fix sendGAEvent function params and type clearly #63226

Open
wants to merge 9 commits into
base: canary
Choose a base branch
from

Conversation

hjick
Copy link
Contributor

@hjick hjick commented Mar 13, 2024

The parameters of the sendGAEvent function type is too ambiguous, the function is working even if entered incorrectly.

I modified the parameters more clearly by referring to the document.

And I add example docs.

Why?

This is GA initialize code.

window['${dataLayerName}'] = window['${dataLayerName}'] || [];
function gtag(){window['${dataLayerName}'].push(arguments);}
gtag('js', new Date());

gtag('config', '${gaId}');`

arguments is array-like object.

gtag('js', new Date()) is gonna be window['${dataLayerName}'].push({ 0: 'js', 1: newDate() })

So, the sendGAEvent function is weird.

Example sendGAEvent in docs now

<button
        onClick={() => sendGAEvent({ event: 'buttonClicked', value: 'xyz' })}
      >
        Send Event
</button>

sendGAEvent({ event: 'buttonClicked', value: 'xyz' })} is gonna be window['${dataLayerName}'].push({ 0: { event: "buttonClicked", value: "xyz" })

But according to GA docs, it has to be window['${dataLayerName}'].push({ 0: 'event', 1: '<event-name>', 2: { <event_parameters> } })
like GA4 event docs example

gtag('event', '<event_name>', {
  <event_parameters>
});

GA4 event docs:
https://developers.google.com/analytics/devguides/collection/ga4/events

issue reference:
#62065 (comment)

@hjick hjick requested review from a team as code owners March 15, 2024 03:33
@hjick hjick requested review from manovotny and leerob and removed request for a team March 15, 2024 03:33
@hjick hjick mentioned this pull request Mar 15, 2024
@hjick hjick changed the title Update GA event function params clearly Fix sendGAEvent function params and type clearly Mar 22, 2024
@@ -54,14 +54,14 @@ export function GoogleAnalytics(props: GAParams) {
)
}

export function sendGAEvent(..._args: Object[]) {
export function sendGAEvent(eventName: string, eventParameters: {[key: string]: string | number}) {
Copy link
Contributor

@jamesmillerburgess jamesmillerburgess Apr 12, 2024

Choose a reason for hiding this comment

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

I think this type is too restrictive.

Some gtag events take an items parameter, which is an array of Item objects.

https://developers.google.com/tag-platform/gtagjs/reference/events

Copy link
Contributor Author

@hjick hjick Apr 12, 2024

Choose a reason for hiding this comment

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

Thank you for your review!

The url you attached return 500 status. Is that url about gtag ```event``` command?

I can't find that items parameters in recent docs about gtag.
https://developers.google.com/tag-platform/gtagjs/reference#event

If the items parameter you're talking about is a gtag command, how about creating a function for each command separately?

Copy link
Contributor Author

@hjick hjick Apr 13, 2024

Choose a reason for hiding this comment

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

I added type GA events include items parameters you referenced.
If the event name entered by the user is the same as the event name in the link you referenced, it also provides autocomplete event parameters.

Can you check this changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think your types won't allow custom events. Maybe there is an existing official set of types from Google that we could use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The type allow custom events. And I also added types from Google docs.

And if user input types from Google official types, user can get autocomplete Google official params.

Also user can input custom types.
I pushed new commit. Please check that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your maintaining. I know you're busy, but could you please check one more time, this commit would be very helpful.

@balazsorban44 balazsorban44 added Documentation Related to Next.js' official documentation. and removed area: documentation labels Apr 17, 2024
@leerob
Copy link
Member

leerob commented Jul 21, 2024

Related: #65678

@samcx
Copy link
Member

samcx commented Jul 25, 2024

@hjick A similar :pr: has been :merge:#66339. Let me know if that :pr: is adequate, or if we should build upon it with this :pr: (not too familiar with the type!)

Also, for whatever reason, I have never been able to access the :google: link. →

CleanShot 2024-07-25 at 13 43 07@2x

@hjick
Copy link
Contributor Author

hjick commented Aug 19, 2024

@hjick A similar :pr: has been :merge:#66339. Let me know if that :pr: is adequate, or if we should build upon it with this :pr: (not too familiar with the type!)

Also, for whatever reason, I have never been able to access the :google: link. →

CleanShot 2024-07-25 at 13 43 07@2x

Sorry, I'm late. I checked that #66339. I think that PR is for type of GTM. I added types for GA. I added all google recommended events (reserved event params).

Right now we can register any value, but I think merging these PRs will help people who aren't familiar with Google's suggested Params by autocomplete them.

Can you enter this link one more?
https://developers.google.com/tag-platform/gtagjs/reference/events?hl=en

@samcx
Copy link
Member

samcx commented Aug 23, 2024

@hjick Thank you! If you can also look into the merge conflict, that will be great :blob_bowing:.

@hjick
Copy link
Contributor Author

hjick commented Oct 6, 2024

@hjick Thank you! If you can also look into the merge conflict, that will be great :blob_bowing:.

Sorry, I'm late. I will fix the conflict soon!

@hjick
Copy link
Contributor Author

hjick commented Oct 27, 2024

@hjick Thank you! If you can also look into the merge conflict, that will be great :blob_bowing:.

I got some problem to fix this conflict errors.. I think there is not enough build error messages.
Can you help me fix this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Related to Next.js' official documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants