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

Ads reload on window resize even if no new sizing rule is triggered #6

Open
johnpuddephatt opened this issue Nov 2, 2015 · 8 comments

Comments

@johnpuddephatt
Copy link

Ads reload each time a window resize is triggered.

At first this seemed to be a minor issue as it only affected users manually resizing the browser window, but I've realised that on iOS/Android a resize can be triggered just by scrolling (because of the address bar minimising for example)

@jcowher jcowher self-assigned this Nov 2, 2015
@johnpuddephatt
Copy link
Author

An extra condition in the if statement in wp-dfp.js on line 54, along with a cache of the sizeMapping object seems to have the desired effect, e.g:

if ( $.fn.dfp && (JSON.stringify(sizeMapping) != JSON.stringify(sizeMappingCache)) ) {
    $ads.dfp( {...} );
    sizeMappingCache = sizeMapping;
}

Not sure if JSON.stringify is the best approach – I read that it's the quickest way to do a simple object comparison, which seems to be what's needed here.

@jcowher
Copy link
Contributor

jcowher commented Nov 2, 2015

Hi @johnpuddephatt,

I just pushed a change to the 1.1.7 branch. Similar idea to what you had, but instead we'll store the value of the viewport width and then when the resize event fires check to see if the width has actually changed before calling init() again. This way changing orientation will also trigger ad resizing. Let me know if this works for you.

@johnpuddephatt
Copy link
Author

That fixes the mobile problem 👍

The thinking behind 'caching' the sizeMapping object was that re-fetching and rendering ads every time the resize event is triggered is inefficient if the sizeMapping object hasn't been altered. It's not a massive concern because window resizing by the user isn't all that common, but it doesn't look great when it does happen.

@johnpuddephatt
Copy link
Author

but it doesn't look great

I should probably mention that I'm mainly referring to the situation where multiple line items are competing for the same slot – in this instance a different creative tends to load each time init() is fired.

@jcowher
Copy link
Contributor

jcowher commented Nov 2, 2015

That's actually a great point. I think, in this case, because sizeMapping is such a simple object we can accurately use JSON.stringify to check for equality. I'll add in your changes to the 1.1.7 branch and I think that'll fix this. Unless you have any additional thoughts?

@jcowher
Copy link
Contributor

jcowher commented Nov 2, 2015

Hey @johnpuddephatt,

Alright, so I just pushed some changes to 1.1.6 (I accidentally jumped ahead to 1.1.7 earlier by accident) that fixes the issues with ads being refreshed when they don't need to be.

I didn't use JSON.stringify, but rather cached the current ad units sizing rule and check to see if it's changed. If it hasn't, the ad unit is removed from the $ads object and then later on we only pass the ads that need refreshing to the $.dfp plugin.

Let me know how you make out.

@johnpuddephatt
Copy link
Author

Sounds like it'll solve the problem and feels like it's the better approach (the fewer ad refreshes the better!)

Only problem is it's giving me a GET error for https://securepubads.g.doubleclick.net/gampad/ads?....

After a bit of digging it looks like the problem is having an ad unit set to display: none in CSS. This wasn't causing a problem before: the ad unit was still included in sizeMapping. Now, however, it's missing from sizeMapping which DFP seems unhappy with.

At the minute I'm not really sure what's going on, I don't see anything in your updated code that handles 'hidden' ad units any differently. When I've got a minute I'll spend a bit more time looking into this and try and figure it out properly.

@PhillipHuynh
Copy link

Hi,

It seems the issue hasn't been fixed for this problem. The ads still reload as I scroll down my iPhone or iPad.

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

No branches or pull requests

3 participants