Hi,

Sorry for bombarding you with issues lately :)

I slightly altered DART functionality (patch attached) related to load_last functionality. The idea is that we render all available Ad tags as settings array in one place instead of creating them inline with the ad unit. The problem with rendering ad calls inline (the way it works now) comes when we load page with AJAX. Basically we load new page on the backend including ad units and then append that page inside AJAX response handler. The problem with that is we are getting js ad tags duplicates. By having all ad calls in settings in one place we can easily make a call to refresh ads for example, or to fill in all ad placeholders.

Not sure if my explanations are clear enough, but hope you will get the idea from the patch itself.

One other thing I did is I changed dart_tag.tpl.php to make it more readable (at least in my opinion) + removed part related to load last, since we have all ad units in settings.

Thanks for your time!

CommentFileSizeAuthor
#21 improve-ajax-v2.patch1.38 KBgansbrest
#20 improve-ajax.patch1.38 KBbleen
#17 dart_load_last.patch1.4 KBgansbrest
dart_load_last.patch3.6 KBgansbrest

Comments

bleen’s picture

bleen commented

hmmm ... I need to digest this.

In the mean time I'll just throw it out there that on at least one site I am using the "load_last" functionality with AJAX without any problems (specifically, we have an ad tag loading within a view so as you use the pager, there is a new ad tag each time a user clicks "next"

.... One other quick note, please open separate issues for separate proposed changes. In this case I may decide that the load_last stuff is great, but not the "cleaner dart_tag.tpl" (or visa-versa) and I cannot review them, commit them separately in that case. I promise I don't mind all the posts lately as they have been very productive (see #1252432: Add a context reaction to change site & zone ) :)

bleen’s picture

bleen commented
Title: Small modifications I made for load_last functionality » When using load_last, consolidate tags into a single settings array

Changing title ... also, I committed a code cleanup for dart_tag.tpl here: #1256292: Cleanup the default dart_tag.tpl.php to use drupal PHP template standards

bleen’s picture

bleen commented

Ok ... I've been digging through this for a while. Is there a site that you can point me to where I can see this in action?

(Any chance you are at DrupalCon london?) If so, I'd love to meetup to discuss... in general I am very often in IRC at #drupal (@bleen) so feel free to ping me about this as well)

bleen’s picture

bleen commented

I also thought of another possible issue with this: what happens if I create a page with one (and only one) ad tag. Then I put a button on it which if clicked, a whole new chunk of html is added to the page including an ad tag. That ad tag will not be present in the js settings ...

Your method will only work when replacing an area of the page that has an ad tag with similar html with the same tag in it. Admittedly, the later is probably more common, but the former is certainly a possibility.

Thoughts?

gansbrest’s picture

gansbrest commented

Ok, lets think about #4. I assume you got one simple page with header and footer, plus one ad tag in the content area somewhere. Presumably you will call dart_tag('tag_name') to render the tag on the page. The page will look similar to this:

<head>
</head>
<content>
  <ad tag wrapper>
    <script type="text/javascript">Drupal.DART.settings.loadLastTags['<?php print $tag->machinename; ?>'] = ..
  </ad tag wrapper>
</content>

When you load new chunk of html (append it to the current content), the content area will look like this:

<content>
  <ad tag wrapper>
    <script type="text/javascript">Drupal.DART.settings.loadLastTags['<?php print $tag->machinename; ?>'] = ..
  </ad tag wrapper>
  <ad tag wrapper>
    <script type="text/javascript">Drupal.DART.settings.loadLastTags['<?php print $tag->machinename; ?>'] = ..
  </ad tag wrapper>
</content>

So basically we are getting settings duplicates.

What I was suggesting in my patch is that we load all ad tags first as js settings in the header section (that way we always have them on the page):

<head>
  [our ads list as js setting]
</head>
<content>
  [just an ad placeholder, we just add extra attributes in the tag_preprocess like data-dart-tag-name="ad-name" so we could replace it with real ad when the page is loaded]
</content>

And when we load new HTML (append to the content), we will get:

  <content>
    [ad placeholder]
    [ad placeholder]
  </content>

The way my patch works now is that Drupal.behaviors.renderDARTtags populates hidden ads placeholders and display them (that way you can call the function multiple times and it will populate just hidden placeholders).

I think we might want to modify that function to support ads refreshing and ads populating. Sometimes you might want to refresh existing ads in response to some action (new slide is loaded for the slideshow etc) and sometimes you want to add more ads on the page and populate just new ones.. We need to think about that I guess..

One other problem I noticed with AJAX ads is that ord and tile are defined in the global scope, so when you render more ads - tile keeps going up and ord stays the same. This may cause problems in some cases, for example when we load new content section which should have all ads starting with tile = 1 plus different ord value to distinguish between pages. Right now I have to manually reset ord and tile after AJAX content is loaded. But this issue could be separated from this thread I guess.

The site I'm working on is in close beta right now so I cant show it to you at the moment, but its going to be pretty crazy. It's a new edition of fastcodesign.com

Unfortunately I wont be attending DrupalCon London this year, but will try to catch you on IRC some time soon. Have fun at DrupalCon!

bleen’s picture

bleen commented

I feel like we are still not on the same page ... here is where my brain is at:

Lets say you have a page with one ad tag with ads.doublecick.com/abc.foo/bar&aaa=bbb

As I understand it if we use your method, then the whole JSON object needed to create that add will be located somewhere in the <HEAD> tag. That seems fine however, lets say that I click a button that ajaxically appends new ad tag like this: ads.doubleclick.com/abc.foo/bar&xxx=zzz ... In this case, the JSON object needed does not exist in the <HEAD> tag because the <HEAD> was done being rendered when the page was first loaded. SOOO, if my button simply brings in an [ad placeholder] as you describe then there will be no data in the <HEAD> to populate it. Am I missing something?

I would love to get more robust handling of ajax ad tags, so lets work this through...

As for the resetting of ord/tile, yes that should be a seperate issue :)

gansbrest’s picture

gansbrest commented

Hmm,

Theoretically you should define all your ad units through DART admin interface before using them. On our site we got 3 ad units defined: Leaderboard, IMU and 88x31. Each one of those ad tags has unique identifier which we use to render the tag with dart_tag() (which will generate placeholder).

Basically we got all those 3 objects in the HEAD section. Then when we load new AJAX content with those 3 ad units - we just populate them from the HEAD section, the same way we populated ad placeholders for the initial page load. We always have all ad units defined through DART interface in the HEAD section.

We may even add additional setting for ad tag admin interface so you could exclude some ad tags from being displayed in the HEAD section. That could be useful for those who has lots of ad units defined through the admin interface.

gansbrest’s picture

gansbrest commented

Do you mean that you wont be able to modify the ad tag (ad remove attributes, change ad zone etc) when doing AJAX request, because all ad tags objects were rendered in the HEAD already? That might be a valid point actually. We need to think about that..

bleen’s picture

bleen commented

RE: #7
I have a site which has 15 dart tags defined ... but only those that appear on the current page would show up in the <HEAD>

In my example in #6, only the one tag placeholder that loads with the page would show up in the <HEAD>, not the any of the other 14 ... but what if the ajax was intended to bring in one of thse other 14 ad tags?

Lets try this example ... lets say you have a slideshow (using views slideshow to ajaxically load each new slide). The page itself has one BANNER ad at the top (hence when the page loads, there is only one JSON object in the <HEAD>). Now lets say the 3rd item in the slideshow has a "sponsored by" logo ad tag ... by your logic, how would the JSON object ever get into the <HEAD>? (note: in this example, both the banner and the logo are predefined in dart settings)

RE: #8
Thats actually a second (potential) problem

gansbrest’s picture

gansbrest commented

With my patch we currently add all ad tags defined in the admin interface to the js setting in the HEAD section. In your case you will get all 15 tag objects on every page. Potentially we could cache that so we could retrieve all of it with one request and then clear cache if you ad new tag, which happens relatively rare I think.

Do you have 15 different active ad tags? We usually create ad tag for each size and then alter attributes, zone, site etc through preprocess function. Usually we don't have more than 6-7 tags defined.

bleen’s picture

bleen commented

Ok .. that was the piece I have been missing. The fact that you add all tags on all pages. Hrmmm

This definitely concerns me and I'm skeptical about caching because if (lets say) you have dart_taxonomy enabled than the tags are different on every node. Plus, that can be a lot of unnecessary data being loaded on each page.

That said, now that we are both on the same page, let me ask you this: why are you loading an entire page via ajax in such a way that it does not replace the existing content on the page? I'm not understanding the use-case I think.

gansbrest’s picture

gansbrest commented

Basically we load the whole section with AJAX, which has Leaderboard and multiple IMU's and append that to the loaded content. You can think of it as "Load more content" functionality, but we load the "whole thing" with artificial header and sidebar :) I know that it's a bit of an edge case, but still..

Maybe we should store the ad tag object inside html "data" attribute and then our render function will process those. That way we don't have to render all tags on the page, plus it would eliminate current settings duplicates, plus you can use preprocess hooks to alter dart tag before it's rendered.

Just thinking..

bleen’s picture

bleen commented

My thought is that generally speaking, this sort of thing should be handled in one of three ways:

1) Your AJAX response should not include ad tags. In most cases this should be pretty able to achieve by adding an extra arg or a query string variable to the request that the AJAX menu callback will recognize and then know not to include the ad tags at all.
2) Your AJAX response should include the ad tags, but they should replace the existing ad tags on the page (at least those in the area you are replacing)
3) Your AJAX response will be appended to the existing page with more ads included. This way a page like facebook's homepage (which grows infinitly) could potentially get more and more ads being loaded.

But you are asking for:
4) Your AJAX response will include ad tags, but you dont want them to actually be displayed

You can already accomplish your suggestion of using an HTML data attribute by simply overriding the dart_tag.tpl.php file in your theme and adding some js ... this does not require any changes to the module. I'm not prepared to do this for the d6 or d7 versions of the DART module because 99% or sites will have a doctype of XHTML and that would invalidate those pages. I will likely do that in the d8 version though, because by then HTML5 will be in core

gansbrest’s picture

gansbrest commented

Hi,

Sorry for late reply. Let say we are talking about 3) from #13. There are couple small issues with current DART implementation:

- when we load ajax response with more tags and append it to the page we will get multiple dart placeholders with the same id.

- at the same time 

        <script type="text/javascript">Drupal.DART.settings.loadLastTags['<?php print $tag->machinename; ?>'] = '<?php print $json_tag; ?>';</script>

will override previous ad call in the array since machine name is the same

- if I call Drupal.behaviors.DART after content was appended to the page it will add ads duplicates to the first matched ad slots. To overcome this I was trying to hide all ad placeholders initially and then process only hidden ones, assuming that the visible ones were already processed and populated with the ad.

As you said I could overcome those issues by modifying template file and creating my own js file for ads refreshing, but I think those are worth mentioning.

bleen’s picture

bleen commented

- when we load ajax response with more tags and append it to the page we will get multiple dart placeholders with the same id.

I dont *think* there is anything inherently wrong with this. Why not display the same ad tag twice here?

at the same time

<script type="text/javascript">Drupal.DART.settings.loadLastTags['<?php print $tag->machinename; ?>'] = '<?php print $json_tag; ?>';</script>

will override previous ad call in the array since machine name is the same

But in hypothetical 3) from #13, thats good. Because if we are appending more content to the page, then we only want the new ad tags to be triggered and thus preventing the problem you pointed to with calling Drupal.behaviors.DART

My brain is beginning to hurt :)

gansbrest’s picture

gansbrest commented

I dont *think* there is anything inherently wrong with this. Why not display the same ad tag twice here?

Well, theoretically element #id should be unique per document. If the element is repeated on the page - it should have class, not id.

By having multiple elements with the same #id Drupal.behaviors.DART will append new ads to the first element with matched id (so you will get multiple ads inside one ad tag) and it's not what we want I guess :)

+ I think it would be nice if Drupal.behaviors.DART would contain some kind of check if ad tag was already populated with the ad, and skip it if it was. That way we could easily call Drupal.behaviors.DART multiple times.

gansbrest’s picture

gansbrest commented
StatusFileSize
new dart_load_last.patch1.4 KB

Can you please consider this simple patch. I think it goes along with your way of thinking (and there is nothing radical about it), plus it suits our needs :)

The idea is that you can easily call Drupal.behaviors.DART method multiple times (from AJAX response handler for example) to populate ad placeholders.

One other problem I noticed with the current approach is that you use the following code 

  $('#block-dart-dart-tag-' + machinename +':visible')

to render ads. But when you render ad tag with dart_tag() function somewhere in the content region, there is no such selector (because it's not a block), which means that placeholder will never be replaced with the actual ad.

Attached patch should fix that issue as well.

Let me know what you think.

bleen’s picture

bleen commented

@gansbrest ... I carved out some time today to really dig in deep on this. I'm still concerned about the idea of including *all* DART tags at the top of every page but for the moment I just wanted to look more closely at how your suggested method works.

Problem is that the patches you've uploaded do not apply against 6.x-2.x-dev ... please reroll if you want me to take another look

gansbrest’s picture

gansbrest commented

Hi

Take a look at #17 patch, I just tried it and it applies cleanly to 6.x-2.x branch. We don't need anything else but this patch for now as I share your concerns about including all DART tags at the top.

The patch attached in #17 doesn't do that. All it does is modifies Drupal.behaviors.DART function so it can be run multiple times to populate ad placeholders after AJAX request, plus it fixes the problem when you render the ad with dart_tag() function in the content area. Current implementation of Drupal.behaviors.DART will skip those placeholders because they are not rendered inside the block and your selector assumes it will contain $('#block-dart part.

Just take a look at patch from #17 and disregard all other patches from this thread.

Thanks a lot.

bleen’s picture

bleen commented
Title: When using load_last, consolidate tags into a single settings array » Improve support for AJAX loaded ad tags by using a "processed" class
Status: Active » Needs review
StatusFileSize
new improve-ajax.patch1.38 KB

@gansbrest ... ok, my post in #18 was me utterly forgetting that git patches have changed the apply process a tiny bit. Sorry about that.

Ok, I have applied #17 and it looks good to me. I made a couple of minor tweaks, specifically adding "dart-" to the new class names. Please double check me and if this still works for you, mark the issue as RTBC and I'm all in favor of this.

gansbrest’s picture

gansbrest commented
Status: Needs review » Patch (to be ported)
StatusFileSize
new improve-ajax-v2.patch1.38 KB

Just checked the patch, there was syntax error because of the accidentally (I think) added quote on the line 517 of the dart.module. I corrected that and new patch (hopefully final) is attached. As for other small modifications you've made I think they totally make sense.

I didn't find how I can change the issue to RTBC, so maybe you can do it for me.

Thanks for your help!

bleen’s picture

bleen commented
Status: Patch (to be ported) » Reviewed & tested by the community

Setting RTBC

I'll commit this to 6.x & 7.x later today

bleen’s picture

bleen commented
Version: 6.x-2.x-dev » 7.x-2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed to dev on both 6.x & 7.x.

Thanks gansbrest!!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.