-
Notifications
You must be signed in to change notification settings - Fork 44
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(adsense)!: remove default data-ad-format
#248
Conversation
@asokawotulo is attempting to deploy a commit to the Nuxt Team on Vercel. A member of the Team first needs to authorize it. |
@@ -37,9 +36,7 @@ const instance = useScriptGoogleAdsense({ | |||
const { status } = instance | |||
|
|||
onMounted(() => { | |||
callOnce(() => { |
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 did do this for a reason π€
Possibly because in dev the HMR was triggering a new ad each time.
We might need to track which ads we've pushed an object for if we're going to remove this
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 see, I'll try play around and see if I can come up with a solution for this
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.
@harlan-zw Just an idea, what if we use callOnce
only in dev, but in production, we push every time the component is mounted.
Something along the lines of:
function pushAdSlot() {
(window.adsbygoogle = window.adsbygoogle || []).push({})
}
onMounted(() => {
import.meta.dev
? callOnce(() => {
pushAdSlot()
})
: pushAdSlot()
// ...
})
@@ -14,7 +14,6 @@ const props = withDefaults(defineProps<{ | |||
*/ | |||
trigger?: ElementScriptTrigger | |||
}>(), { | |||
dataAdFormat: 'auto', |
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 okay with this change but as it's a breaking change it would need to wait until 0.9.0 so maybe better to have it in a separate PR.
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.
Alright let me revise the PR
data-ad-format
and allow for more than one ad slot on the pagedata-ad-format
π Linked issue
Resolves #247
β Type of change
π Description
Removed the default ad format, allowing for fixed-size ad units