CSS classes and theming changed [#2061423]

Hi,
First, thanks for the new 6.x-1.5 release. However, after upgrading from 1.4 I found that the styling of my ad blocks was not the same as before. I have tracked this down to three specific things, which I think should be shared, hence I am reporting them here.

In adsense_managed the block now has its own theme template file block-adsense_managed.tpl.php which takes precedence (at least in my case) over the default theme block.tpl.php for the sites theme. It is easy to delete this file, if you want your existing theme files to be used.
In the function theme_adsense_ad() it looks like you have a typo where $module_ should be $module. This other variable does not exist, so the class of the div is missing. Maybe this could be fixed?
In the new function theme_adsense_placeholder() which replaces _adsense_format_box, the class of adsense-placeholder is used, when previously it was just adsense. This is a useful change, but needs to be highlightled in case users (like me) were using 'adsense' in our own .css files

Hope that helps others who find their ad blocks looking different to before.

Jonathan

Comments

Comment #1

jcnventura

English

commented 9 August 2013 at 23:14

Hi Jonathan,

Replying to your points:

1. Do NOT delete the file.. The objective of that file is to make sure that the standard drupal block naming conventions are not applied to the block. This is because the most popular ad-blocking lists contain a rule that targets any #block-adsense_managed* blocks, and prevent the display of that whole block. Even if it contains only the experimental 'please disable ad blocking on this site', request. If you've modified block.tpl.php in your theme, I think you can also create a theme-specific block-adsense_managed.tpl.php file.

2. Not a typo.. In my test envs, this effectively appends an underscore to the module name. So "adsense_managed" becomes "adsense_managed_". Which does not match any of the ad-blocking rules. However the proper way to do this is to use {$module}_ since the underscore can also be part of the variable name. As it works in my envs it doesn't seem critical but I'll fix it soon(ish).

3. My bad. I tried to remove the inline styles in that function and move them into a CSS file. That didn't work because the css file was being ad-blocked. So I reverted back to inline styles and got rid of the CSS file, but never changed the class to the previous definition. I'll re-add the simple adsense in addition to the placeholder class.

João

Comment #2

jonathan1055 commented 10 August 2013 at 09:04

Hi, thanks very much for your answers. I was unaware of all the ad-blocking work you have done, and now I see why you have changed things.

OK I wont delete the .tpl file. I saw that it had an altered set of classes, and wondered why. I can probably edit the sites own theme .css files to add the styling required (padding, margins, etc) which it does for normal blocks.
That's interesting that your test site can create the value 'modulename_' for $module_ because I thought that php would only try to resolve the variable $module_ and because it does not exist, would not do anything else. For class='adsense $module_' I get just class='adsense ' whereas you say that because $module is 'adsense_managed' you get class='adsense adsense_managed_'. Are you really sure?
It is fine just to have just adsense-placeholder class, but if you want to have both that is ok too.

Thanks again for your explainations, and for doing great work on the module.

Jonathan

Comment #3

jcnventura

English

commented 16 August 2013 at 22:37

Hi Jonathan,

Yes, I'm sure about it. We probably have different PHP versions.. I'll keep this open until I do the minor modifications that I indicated in points 2 and 3.

João

Comment #4

muckermarc commented 20 February 2014 at 13:05

Issue summary:

View changes

Hi,

I have a similar issue with this module, in that I need to style up the blocks, so ideally I'd like a class attribute in the block div tag.

1. Do NOT delete the file.. The objective of that file is to make sure that the standard drupal block naming conventions are not applied to the block. This is because the most popular ad-blocking lists contain a rule that targets any #block-adsense_managed* blocks, and prevent the display of that whole block. Even if it contains only the experimental 'please disable ad blocking on this site', request. If you've modified block.tpl.php in your theme, I think you can also create a theme-specific block-adsense_managed.tpl.php file.

Based on this comment, I figured I could copy block-adsense_managed.tpl.php into my own theme, and edit the file to drop in an extra class attribute:

<div id="block-<?php print '-'. $block->delta; ?>" class="block adsense_managed">
<?php if ($block->subject): ?>
  <h2><?php print $block->subject ?></h2>
<?php endif;?>

  <div class="content">
    <?php print $block->content ?>
  </div>
</div>

But despite clearing the theme registry, it doesn't seem to pick up my template file and reverts to the original block-adsense_managed.tpl.php. So for now, I've had to edit the original block-adsense_managed.tpl.php, but this isn't ideal.

Any ideas on why the template over-ride aint working?

Thanks,

Marc

Comment #5

jcnventura

English

commented 20 February 2014 at 22:52

Status:

Active

» Postponed (maintainer needs more info)

Your theming setup probably doesn't work to theme other blocks as well. However it's also possible that the hook_theme_registry_alter() needs to be improved.

Can you try to make sure you can theme other blocks?