• You have a lot of css in theme_google_plusone_badge() and google_plusone_block_configure() - could that be refactored into a .css file and included with drupal_add_css()? That seems like it would be a lot cleaner and allow it to be aggregated.
  • All the google_plusone_badge_* variables are not being deleted in hook_uninstall().
  • In google_plusone_page_alter(), in the case of !$async, can you load the js file with drupal_add_js instead? A nice feature is letting admins provide the js file locally so it can be aggregated, compressed and cached.

That's all I could find, looks like a well implemented module!

Comments

corbacho’s picture

corbacho CreditAttribution: corbacho commented

Hey Karl. Thank you for this great review!. I appreciate it a lot
I will for sure follow these advices ASAP and fix the issues that have been opened

Thank you again

corbacho’s picture

corbacho CreditAttribution: corbacho commented

I had some time to fix some bugs and have a look at your review.

* About the CSS: I'm happy to see that Google has changed how is generating the CSS and markup of the badges markup/css has changed. https://developers.google.com/+/web/badge/#badge
Now is much cleaner, so that CSS it will be removed.

* Thanks for that bug. Now hook_uninstall is removing *All* the module variables. (Now in dev branch 7.x.-1.x)

* I can't use drupal_add_js because of really special way that google wants you to add their JavaScript:

<script type="text/javascript" src="https://apis.google.com/js/platform.js">
  {lang: 'et'}
</script>

You see that "lang" option is inside of the script tag. This impossible to do it with drupal_add_js()

I will mark as fix later on, when I remove that CSS