[D7] AdRoll Smartpixel Integration

Hey @skh,

Thanks for taking out the time and reviewing my module :)

Replies inline -

You have some pareview errors,

Fixed and commited them to the repo. Here a run of the tests again - http://pareview.sh/pareview/httpgitdrupalorgsandboxsghosh2270117git

The code in the description for cloning the repository is for your personal d.o account, it should be git clone --branch 7.x-1.x http://git.drupal.org/sandbox/SGhosh/2270117.git,

Updated.

Consider adding the information in the read me to your project's overview page,

But currently the module is sandbox. Should I not add this information after I have a full project page for this?

You do not need version = 7.x-1.0 in your .install file (as per #542202).

Removed.

Hey artsakenos,

thanks a lot for you feedback :)

First of all i’d like to say that Drupal blocks and boxes in full html configuration already allow to paste the Smartpixel script without actually coding, rather just changing the personal parameters.

Yes that is a possible solution, but even with that user can place blocks only in a given list of regions as defined by their theme. Smartpixel requires the js to be added just before the closing body tag, as in here, under 'Summary'
Hence, this module, to ensure the js is added in the html as specified by adroll smartpixel.

The git clone command must be:
git clone --branch 7.x-1.x git.drupal.org:sandbox/SGhosh/2270117.git adroll_smartpixel_integration
Your userid must be removed, and also you would directly create the module folder with the correct name.

Fixed.

Please refer to this useful template to the compile the README.TXT file according to the guidelines.

Updated. Thanks, really good template to follow, did not know about it.

It would be nice to see in the description of your module a link and a brief tagline about the Adrool service or the Smartpixel code snippet.

Added a brief about the AdRoll Smartpixel along with a link to the site, and also some information about the way to configure the module.

Consider moving the admin panel hooks inside a .admin.inc file, keeping only the core inside the module (the variable settings).

Moved the callback to an .inc file. Thanks for pointing this out.

Documentation is incomplete in some case (e.g., Stores the …), please check.

I have updated both the README file as well as the documentation on the module page.

Thanks again for your time. Some really good improvements in my module now from your feedback :)

Hey hurley,

thanks a ton for the review and the valuable suggestions. Please find my replies inline -

You should be using Drupal.behaviors to call your javascript. See https://www.drupal.org/node/304258

The Js is supposed to go directly into the html body as per AdRoll instructions. However since I wanted to put the JS through JS file, hence I put the exact JS directly into the JS file. Also, Drupal.behaviors is used mostly to re-add JS code to html loaded through ajax, however this code is supposed to go into the element and only once, I did not think there would be any requirement of a re-addition on any ajax load. Yet, do you think Drupal.behaviors should be the right way of doing it? What advantage will I get from using it?

You may want to add a hook_help. This would be useful for site builders who may not be looking at the README.txt.

Added almost the same data as I have in my README.txt.

You may also want to add a link to AdRoll in your README.txt and hook_help
It's also nice to include configure in your .info file: configure = admin/config/search/adroll. This will add a configure link on the modules page which is is nice.

Added.

Hey tkuldeep17 ,

thankyou so much for reviewing my module :). Please see the code below to see where I am using the settings data -

function adroll_smartpixel_integration_init() {
  // Add AdRoll js only on non admin pages.
  if (arg(0) != 'admin') {
    $adroll_adv_id = variable_get('adroll_smartpixel_integration_adv_id', '');
    $adroll_pix_id = variable_get('adroll_smartpixel_integration_pix_id', '');
    // Add the AdRoll js only if the required IDs have been provided.
    if ($adroll_adv_id && $adroll_pix_id) {
      drupal_add_js(
        array(
          'adroll_smartpixel_integration' => array(
            'adroll_adv_id' => $adroll_adv_id,
          ),
        ),
        'setting'
      );
      drupal_add_js(
        array(
          'adroll_smartpixel_integration' => array(
            'adroll_pix_id' => $adroll_pix_id,
          ),
        ),
        'setting'
      );
      drupal_add_js(
        drupal_get_path('module', 'adroll_smartpixel_integration') .
        '/adroll_smartpixel_integration.js',
        array('scope' => 'footer', 'weight' => 1000)
      );
    }
  }
}

As you can see above, in my init function, I am fetching the settings data and if and only if both provided, adding the relevant js file for adroll integration and also passing the settings data to the js file for use in the adroll js code.

Please let me know if you have any more questions. Once again, thanks for the review :)

Hey tkuldeep17,

thanks for the quick reply. Please see the php and js code below -

$adroll_adv_id = variable_get('adroll_smartpixel_integration_adv_id', '');
$adroll_pix_id = variable_get('adroll_smartpixel_integration_pix_id', '');
...
drupal_add_js(
        array(
          'adroll_smartpixel_integration' => array(
            'adroll_adv_id' => $adroll_adv_id,
          ),
        ),
        'setting'
      );
      drupal_add_js(
        array(
          'adroll_smartpixel_integration' => array(
            'adroll_pix_id' => $adroll_pix_id,
          ),
        ),
        'setting'
      );

A you can see above I am passing the 2 variables to js - adroll_adv_id and adroll_pix_id. And below is the Js code where I am accessing these variables.

adroll_adv_id = Drupal.settings.adroll_smartpixel_integration.adroll_adv_id;
adroll_pix_id = Drupal.settings.adroll_smartpixel_integration.adroll_pix_id;

I hope I was able to clear your doubt this time :)

@tkuldeep17, also updated the git clone command in the my description. Thanks!

+klausi, recently I came across this fantastic module - Tracking Code. It allows you to add any js code to the body and even specify whether you want it in head, just after the starting body tag or just before the closing body tag. Not only that, you can also specify the pages you want the code on.
Hence, that module will satisfy not only this requirement(ie. placing alexa code in the html) but also any other such js code placement requirement. So it was only intelligent on my side to stop working on this module and if I have some ideas maybe contribute to that module itself. After all we are aiming for collaboration over competition, right?! :)

Closing this in favour of Tracking Code.