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: googletag.display errors when ad slot not in DOM #142

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

caffeinated-pixels
Copy link
Contributor

@caffeinated-pixels caffeinated-pixels commented Oct 24, 2024

Description

This PR fixes an issue where GPT complains if googletag.displaySlots() is called for a defined ad slot that is not yet in the DOM, i.e.

GPT] Error in googletag.display: could not find div with id "my-ad-id" in DOM for slot: /my/ad/unit/path
  • The fix is to defer the calling of Advertising.displaySlots(). So instead calling this method in Advertising.setupGpt(), we are now calling it at the end of Advertising.active().
  • I've also refactored displaySlots() to accept an id and to only call window.googletag.display(id) for this single id, instead of iterating through all the slots
  • Advertising.active() gets called from the AdvertisingSlot component, so now Advertising.displaySlots() will not fire for defined ad slots that are not yet in the DOM.
  • The solution was based on a suggestion from @Trav84, as outlined in: https://github.com/orgs/KijijiCA/discussions/141

@caffeinated-pixels caffeinated-pixels marked this pull request as ready for review October 24, 2024 19:55
@caffeinated-pixels caffeinated-pixels requested a review from a team as a code owner October 24, 2024 19:55
@caffeinated-pixels caffeinated-pixels requested review from masonbraun, makandz and thedaviddias and removed request for a team October 24, 2024 19:55
@rcedev
Copy link

rcedev commented Dec 15, 2024

Is there a reason this hasn't been merged? I'm getting the same console error messages, where GPT is attempting to display an ad before the div element has been rendered to the DOM.

@caffeinated-pixels
Copy link
Contributor Author

Is there a reason this hasn't been merged? I'm getting the same console error messages, where GPT is attempting to display an ad before the div element has been rendered to the DOM.

We were waiting on @Trav84 to test locally, but didn't hear back, so it kinda got forgotten about. From my own local testing, it seemed to work as expected but it would be good if someone else was also able to test.

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.

2 participants