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

Delayed display of configured slots when working with infinite scrolling #130

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

spossner
Copy link

Description

When working with a larger amount of slots which are not in the dom from the very beginning, the current (non dynamic configuration) approach defines and displays all configured slots when setting up gpt. Due to GPTs API reference, slots must be in the dom before calling display. This is violated in the scenario above and ends in console warning messages.

Solution / Proposal

I have introduced a delayed display "list" which stores all IDs of AdvertisingSlots which are defined (in setupGpt) but not yet found in the dom.
When it comes to activation, such slots are displayed before doing the GPT refresh.
Therefore I have refactored the machinsm into a separate displayAndRefreshSlots function

(I have also upgraded the node version to LTS in order to avoid an outdated ssl issue in webpack plus bumped some package versions to avoid vulnerabilities)

@spossner spossner requested a review from a team as a code owner May 30, 2023 07:13
@spossner spossner requested review from MisterJimson and makandz May 30, 2023 07:13
@spossner spossner marked this pull request as draft May 30, 2023 15:38
@spossner
Copy link
Author

Just realized that the tests are not executed while building - and they break because the display method of GPT is not called anymore if div is not available in dom (as it probably is the case for the jest tests). Need to review that first. -> moved PR back in draft

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.

1 participant