Support of responsive ads

I'm looking at this exact same thing and will try and get a patch together this week.

/me very interested in patch

4: dfp-add-adaptive-ads-2133053-8170983.patch queued for re-testing.

I would advise against harcoding the breakpoints into the module.

We should let the user supply an ad unit size and corresponding breakpoint like shown in the example from google: https://support.google.com/dfp_premium/answer/3423562?hl=en

Attached patch adds a section to the tag edit form. It functions almost exactly like the targeting, you can add multiple "breakpoints".

This patch does NOT refresh the ads on browser resize though. Because the resize event fires so many times while the browser is being resized I think something like this: http://stackoverflow.com/a/12692647/1316803 should be used. I'm doing that in my theme, but wonder if it should be included in this patch?

Something like:

$(window).resize(function(){
    if(this.resizeTO) clearTimeout(this.resizeTO);
    this.resizeTO = setTimeout(function() {
      $(this).trigger('resizeEnd');
    }, 500);
  });

  $(window).bind('resizeEnd', function() {
    googletag.pubads().refresh();
  }

I do notice one other issue with responsive ads. In my case I was displaying a background takeover ad, except at the mobile size. I was using a 730x200 breakpoint with an ad size of 2000x2000. In order to not display on smaller screens, I had to change the "Size(s)" in the "Tag Settings" section to 0x0. This works properly when the browser starts at less then 730px wide. When you resize to larger and the ads are refreshed, the background ad shows up, as I would expect. However, if you resize back down to <730px wide, the ad still shows up. It looks like this is a known issue and a planned update:

Responsive GPT phase 2 will add the ability to suppress (not show) ads for certain browser sizes.

from https://support.google.com/dfp_premium/answer/3423562?hl=en

Last, this patch probably won't apply cleanly to 7.x-1.1 because of changes to _dfp_js_slot_definition(), so you may need to apply some of the changes manually if you are not using the dev version.

An overarching question first: is there any reason why we should or shouldn't leverage the breakpoint module for the vast majority of this functionality? I have never used it so I am genuinely asking ... could we? should we?

Assuming the answer to that is "thats a bad idea" ... below are some comments. Most are small. By-in-large I think this is a good patch... Another overarching thought though... is there a way to simplify the UI in terms of "size" vs. "responsive settings" ... in a way, they are asking for the same thing, its just that "size" is the default. Thoughts on this?

1. +++ b/dfp.admin.inc
   @@ -422,6 +422,167 @@ function _dfp_targeting_remove_empty(&$target) {
   +function dfp_breakpoint_form_validate($element, &$form_state) {
   

   Should we also be checking for valid screen sizes here? Ex. 1024xhotdog is invalid. Not sure if things will break in this case

2. +++ b/dfp.admin.inc
   @@ -422,6 +422,167 @@ function _dfp_targeting_remove_empty(&$target) {
   + * Helper function that takes a form_state['values'] and removes empty breakpoints.
   

   nit: Comments should break to the next line if they are more than 80 chars

3. +++ b/dfp.admin.inc
   @@ -422,6 +422,167 @@ function _dfp_targeting_remove_empty(&$target) {
   +  $breakpoints_form['breakpoints'] = array(
   +    '#type' => 'markup',
   +    '#tree' => FALSE,
   +    '#prefix' => '<div id="dfp-breakpoints-wrapper">',
   +    '#suffix' => '</div>',
   +    '#theme' => 'dfp_breakpoint_settings',
   +    '#element_validate' => array('dfp_breakpoints_form_validate'),
   

   I think this element needs some descriptive text about what is going on there. "Hey, I just set a size for this ad slot two tabs ago... how is this different?"

4. +++ b/dfp.module
   @@ -49,6 +49,10 @@ function dfp_theme($existing, $type, $theme, $path) {
   +      'file' => 'dfp.admin.inc',
   

   I'm not sure I'm right about this, but is it worth considering putting all the breakpoint stuff in a separate .inc file? Meh...

Added re-rolled patch.

• Addressed missing validation for non number, x, comma
• Changed summary, added description for section with reference to documentation
• Fixed long comment

Didn't take a look at breakpoint yet but this patch should get it up and running.

Hold off on this, found a bug. Let me re-roll

OK all set, mapping working for me on my local

+++ b/dfp.admin.js
@@ -75,6 +75,11 @@ Drupal.behaviors.dfpVerticalTabs = {
+      var summary = 'Configure DFP mappings.';
+      return summary;
+    });

summary is missing "$" on both lines

@bleen18 that's the javascriptz, not the phpz

Something was wrong with the last patch, marked as corrupt. Re-rolled and tested against 7.x-1.x

Mas patches

Per #12 this patch does NOT refresh the ads on browser resize. You can look the js in 12 if you would like to try that. I don't think it is a good idea to implement here.

As for breakpoint, I couldn't get a real fix on the state of the module with the current threads, I think getting this in is a pretty easy win for mappings.

ok ... I'm pretty happy with #25 but before I commit it, this needs some simpletests. We should be testing a) the validation function works properly with different values and b) that given several breakpoints, the correct JS is displayed in the markup. Once we have that, I think we are good to go...

I assume the comment in #13 meant the "breakpoints" module, rather than "breakpoint".
https://drupal.org/project/breakpoints

I am currently using the picture module with the breakpoints module. It seems like it would be a good example of usage for anyone evaluating the breakpoints module.
https://drupal.org/project/picture

Interesting, I would look into this if everyone agrees it is the best path. It will create a module dependency for DFP if we go down that path. Let me know your thoughts @bleen18. I was going to tackle the unit tests this week.

I am very interested in using the break points module to manage the BPs ... Less code to babysit:)

It shouldn't have to be a dependency though. Use if module_exists where needed and voila.

I have the patch up and running in our latest staging environments without issue. I'd mark as RTBC, but I see you are waiting on tests to be written.

This patch works perfectly, great work, thanks to everyone who worked on this.

I just applied the patch, set the breakpoints for 2 sizes, refreshed my page and bingo, 2 different ad sizes for 2 different screen sizes.

I then exported to features, and all seems well.

Added requested tests. I think integrating with breakpoints should be another issue, this is working for us.

Sold!!

Committed to 7.x-1.x

submodule for responsive dfp ads.

The example code here https://support.google.com/dfp_premium/answer/3423562 shows how to exclude ads from appearing within certain breakpoints.

// The following mapping will only display ads when the user is on a desktop sized viewport.
        var mapping1 = googletag.sizeMapping().
            addSize([0, 0], []).
            addSize([1050, 200], [1024, 120]). // Desktop
            build();

With the current development version I can verify that if you leave the ad dimensions field empty it will indeed exclude the ad from displaying within those breakpoint dimensions. I had to comment out the code in the validation hook to be able to leave the field blank.

I'd like to contribute a fix to allow excluded breakpoints, what would be the best approach? I can open a new ticket if that would be more appropriate.