Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Allow DFP async slots to accept multiple ad sizes #82

Merged
merged 13 commits into from Sep 15, 2015

Conversation

Projects
None yet
7 participants
Contributor

harvitronix commented Jul 23, 2013

I've added the option to define your ad unit sizes as an array of sizes to account for flex units. A situation where this would be important is if someone is trying to run both 728x90 and 970x66 sized units through the same header ad unit. (Or 300x250 and 300x600 ads through the same above the fold right rail unit.)

To define such an ad unit, you add a 'sizes' key to the url_vars array in the acmx_filter_ad_tag_ids ( $ids ) filter/array.

An example of how to define a flex space:

array(
            'tag'       => 'dfp_desktop_top',
            'url_vars'  => array(
                'tag'       => 'dfp_desktop_top',
                'sizes' => array(
                    array(
                        'width'     =>  '728',
                        'height'    => '90'
                    ),
                    array(
                        'width'     =>  '970',
                        'height'    => '66'
                    ),
                ),
            ),
            'enable_ui_mapping' => true
        ),

This will create an ad slot that looks like:
googletag.defineSlot('/XXX/728x90_flex', [[728,90],[970,66]], "div-id").addService(googletag.pubads())

This update accounts for single-sized configurations as well and thus is backwards compatible with the current async tag ids array.

Contributor

NRG-R9T commented Jul 24, 2013

isn't it close to the solution, that will allow responsive ads? see this: http://exisweb.net/how-to-use-google-adsense-on-a-responsive-website the hack is appproved by google too.

Contributor

harvitronix commented Jul 24, 2013

@NRG-R9T DFP async tags allow you to define multiple sizes for the same slot, natively. This pull request allows you to define these multiple sizes in ACM's tag id array. It negates the need for the responsive hack (and having to create a new div for each ad size) and provides native DFP functionality through ACM, which I believe is advantageous to users.

Contributor

harvitronix commented Jul 24, 2013

@NRG-R9T I also just realized these are two separate issues. The hack is for serving different ad unit sizes depending on viewport sizes. My solution is for allowing DFP to run different sized creatives through the same ad unit, regardless of viewport.

natebot commented Jul 24, 2013

Yes, the support for DFP's new syntax for creative sizes is different and arguably easier to tackle. The lack of this support for DFP's new ad dimension syntax is one of the things keeping us from adopting the plugin at Grist. I'm happy to contribute/test to see this support added.

On Wednesday, July 24, 2013 at 8:25 AM, Matt Harvey wrote:

@NRG-R9T (https://github.com/NRG-R9T) I also just realized these are two separate issues. The hack is for serving different ad unit sizes depending on viewport sizes. My solution is for allowing DFP to run different sized creatives through the same ad unit, regardless of viewport.


Reply to this email directly or view it on GitHub (#82 (comment)).

Contributor

NRG-R9T commented Jul 26, 2013

@harvitronix So it is a step forward, thank you. For us at greenbyte.ch the lack of DFP support for responsive ads is a pain unsolved by this ACM-plugin, until custom viewports are supported. thanks @natebot for the feedback. can we solve this together?

Contributor

swissspidy commented Feb 7, 2014

What's the current state of this pull request? Are there still plans to integrate it?

Collaborator

danielbachhuber commented Feb 7, 2014

I'm open to a merge provided someone thinks the PR is complete and can provide test cases.

+1 for this PR

@jeffstieler jeffstieler added a commit to voceconnect/Ad-Code-Manager that referenced this pull request Apr 28, 2015

@jeffstieler jeffstieler Automattic#82 c3b36ba
Collaborator

rinatkhaziev commented Sep 15, 2015

Wow, this is long overdue, merging.

@rinatkhaziev rinatkhaziev added a commit that referenced this pull request Sep 15, 2015

@rinatkhaziev rinatkhaziev Merge pull request #82 from harvitronix/flex-sizes
Allow DFP async slots to accept multiple ad sizes via 'sizes' variable
c0e2b86

@rinatkhaziev rinatkhaziev merged commit c0e2b86 into Automattic:master Sep 15, 2015

Contributor

harvitronix commented Sep 15, 2015

@rinatkhaziev This has been open so long I completely forgot about it. Although this code technically works, there are two issues:

  1. The $unit_sizes_output should be created as an array and then json encoded. As I originally wrote it a couple years ago, I manually create a json-style string. Messy.
  2. This bypasses an ad size filter. I haven't had a chance to dig into the details, but from memory, I know we had this problem before.

Apologies for not commenting on these issues earlier.

Collaborator

rinatkhaziev commented Sep 15, 2015

  1. Yeah I thought about it, are you open to rework it and commit the changes, so that parse_ad_tag_sizes returns an array of arrays with width and height, and then do wp_json_encode( $unit_sizes)?
Contributor

harvitronix commented Sep 15, 2015

Can do.

Collaborator

rinatkhaziev commented Sep 15, 2015

@harvitronix awesome 👍

Collaborator

rinatkhaziev commented Sep 15, 2015

@harvitronix actually, nevermind, i'll just do it myself, since i'm already working on PRs/readme updates now.

Contributor

harvitronix commented Sep 15, 2015

@rinatkhaziev Done here: #99

Cheers!

Collaborator

rinatkhaziev commented Sep 15, 2015

@harvitronix ah, great! thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment