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

feat(GA,GTM): configurable dataLayer name #163

Merged
merged 10 commits into from
Jul 21, 2024
Merged

feat(GA,GTM): configurable dataLayer name #163

merged 10 commits into from
Jul 21, 2024

Conversation

huang-julien
Copy link
Member

@huang-julien huang-julien commented Jul 18, 2024

πŸ”— Linked issue

#154

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Update to tpc 1.1.1 with minor changes.

  • adds a new dataLayerName option
  • on our side we handle theses options and we define defaults for dataLayerName to avoid conflicts between gtm and ga
    • defaultGa for google analytics
    • defaultGtm for google tag manager

Copy link

vercel bot commented Jul 18, 2024

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Comments Updated (UTC)
scripts-docs βœ… Ready (Inspect) Visit Preview πŸ’¬ Add feedback Jul 19, 2024 8:55pm
scripts-playground βœ… Ready (Inspect) Visit Preview πŸ’¬ Add feedback Jul 19, 2024 8:55pm

@huang-julien huang-julien marked this pull request as draft July 18, 2024 23:10
Copy link
Contributor

@flashdesignory flashdesignory left a comment

Choose a reason for hiding this comment

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

LGTM, assuming the imports are good as is πŸ‘

@@ -13,32 +12,27 @@ export interface TpcDescriptor {
tpcTypeImport: string
key: string
registry?: any
options: ({
Copy link
Collaborator

@harlan-zw harlan-zw Jul 20, 2024

Choose a reason for hiding this comment

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

any particular reason to change this? this matches the useRegistryScript interface so keeps code closer aligned


declare global {
interface Window extends GoogleTagManagerApi {}
}
export type GoogleTagManagerInput = RegistryScriptInput<typeof GoogleTagManagerOptions>

export function useScriptGoogleTagManager<T extends GoogleTagManagerApi>(_options?: GoogleTagManagerInput) {
_options = defu(_options, { dataLayerName: 'defaultGtm' })
return useRegistryScript<T, typeof GoogleTagManagerOptions>(_options?.key || 'google-tag-manager', options => ({
scriptInput: {
src: withQuery('https://www.googletagmanager.com/gtm.js', { id: options?.id }),
Copy link
Collaborator

Choose a reason for hiding this comment

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

the src needs to include the dataLayer name otherwise how will it link it πŸ€”

Copy link
Member Author

Choose a reason for hiding this comment

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

hmmm it's not in TPC πŸ‘€ . Is there a default name for data layers ?
cc @flashdesignory

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh wait I just merged this and I forgot I left this comment.

Can we do some testing on it and make sure it's actually working? I'm pretty sure it will just use dataLayer if not configured.

cc: @flashdesignory

@harlan-zw
Copy link
Collaborator

harlan-zw commented Jul 20, 2024

Personally, I'd prefer to see the default behaviour to use window.dataLayer, I think it's more intuitive for end users if they really need to pull for the underlying implementation.

Is using dataLayers an existing best practice otherwise? A non-nested key would also simplify things, just using directly what the user provides. Again this is more intuitive for end users, we want to avoid hiding implementation details.

@flashdesignory
Copy link
Contributor

Personally, I'd prefer to see the default behaviour to use window.dataLayer, I think it's more intuitive for end users if they really need to pull for the underlying implementation.

Is using dataLayers an existing best practice otherwise? A non-nested key would also simplify things, just using directly what the user provides. Again this is more intuitive for end users, we want to avoid hiding implementation details.

The idea of the window.dataLayers object was to keep it scoped, but it is less popular, compared to just assigning custom names directly to the windows object.

we can revert back to the previous implementation, where we'd use window.dataLayer, unless we get another dataLayer name passed in. TPC, will expect a value from the wrapper (even if it's the default name).

@harlan-zw
Copy link
Collaborator

harlan-zw commented Jul 21, 2024

Oh my mistake, I didn't realize this is how it was baked into TPC. We can go with this solution to move this along.

Some doc for end-users on which window var is set may be useful.

@harlan-zw harlan-zw changed the title feat(tpc): update to 1.1.1 feat(GA,GTM): configurable dataLayer name Jul 21, 2024
@harlan-zw harlan-zw merged commit 95bac7c into main Jul 21, 2024
4 checks passed
@harlan-zw harlan-zw deleted the feat/up_tpc branch July 21, 2024 03:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants