-
Notifications
You must be signed in to change notification settings - Fork 8
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
add script types and a simple type guard #27
add script types and a simple type guard #27
Conversation
@@ -15,3 +15,5 @@ | |||
export { GoogleAnalytics } from './third-parties/google-analytics'; | |||
export { GoogleMapsEmbed } from './third-parties/google-maps-embed'; | |||
export { YouTubeEmbed } from './third-parties/youtube-embed'; | |||
|
|||
export * from './types'; |
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.
just a way to get the types exported...
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.
My TS skills are not as good as I'd like them to be lol
Is this how folks generally export types? Do you know if packages usually includes a types file as well?
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.
there's maybe a more "proper" way, this just made it work for me.
@janicklas-ralph - what did you have in mind?
would be nice maybe to be able to import them from "third-party-capital/types" instead?
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.
Yeah tbh I'm not sure, but we can figure that out in a future PR :)
@@ -11,7 +11,7 @@ | |||
"action": "append" | |||
}, | |||
{ | |||
"code": "window.dataLayer=window.dataLayer||[];window.gtag=function gtag(){window.dataLayer.push(arguments);};gtag('js',new Date());gtag('config','${args.id}')", | |||
"code": "window.dataLayer=window.dataLayer||[];window.gtag=function gtag(){window.dataLayer.push(arguments);};gtag('js',new Date());gtag('config','{{id}}')", |
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.
using a more traditional template tag
@@ -38,6 +39,12 @@ export function formatUrl( | |||
return newUrl.toString(); | |||
} | |||
|
|||
export function formatCode(code: string, args?: Inputs) { |
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 function replaces the template tags with args passed in
@@ -11,7 +11,7 @@ | |||
"action": "append" | |||
}, | |||
{ | |||
"code": "window.dataLayer=window.dataLayer||[];window.gtag=function gtag(){window.dataLayer.push(arguments);};gtag('js',new Date());gtag('config','${args.id}')", | |||
"code": "window.dataLayer=window.dataLayer||[];window.gtag=function gtag(){window.dataLayer.push(...arguments);};gtag({'gtm.start': Date.now(), 'event': 'gtm.js'});gtag({'config':'{{id}}'})", |
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.
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.
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.
weird thing is that if we call the gtag function (either snippet or generated), it's basically an array, vs the automatic events fired are objects 🤷 .
I did compare with https://developers.google.com/tag-platform/gtagjs/configure and it looks right.
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.
Just one nit, otherwise this is great ❤️
Thanks for doing this!
@@ -11,8 +11,7 @@ | |||
"action": "append" | |||
}, | |||
{ | |||
"code": "window.dataLayer=window.dataLayer||[];window.gtag=function gtag(){window.dataLayer.push(arguments);};gtag('js',new Date());gtag('config','${args.id}')", | |||
"strategy": "worker", | |||
"code": "window.dataLayer=window.dataLayer||[];window.gtag=function gtag(){window.dataLayer.push(arguments);};gtag('js',new Date());gtag('config','{{id}}')", "strategy": "worker", |
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.
Nit: Identation
I thought I already set up Prettier in this package, but maybe I need to take another look 🤔
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.
🤦♂️ - fixed
@@ -15,3 +15,5 @@ | |||
export { GoogleAnalytics } from './third-parties/google-analytics'; | |||
export { GoogleMapsEmbed } from './third-parties/google-maps-embed'; | |||
export { YouTubeEmbed } from './third-parties/youtube-embed'; | |||
|
|||
export * from './types'; |
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.
Yeah tbh I'm not sure, but we can figure that out in a future PR :)
just a starting point... could be exported differently, but at least it works.
with this we can do:
@kara