Twitter Block currently uses the HTML returned by the twitter_block_load_tweets() function as block content. This causes a few issues:

  • Twitter Block includes the twitter_block.css file in the module's .info file. This unnecessarily includes the CSS on every page which negatively affects performance. Attaching the CSS to the block's content inside of a render array will only load the CSS when the block is displayed.
  • The unordered list of tweets displayed in the block is wrapped in a string of HTML. This HTML is difficult to modify compared to adding or removing a class from the class attribute of a render array.
  • The twitter_block theme function is primarily provided for users to customize the wrapper around the tweets. By integration this into the render array, the theme function can be removed which would improve performance.
CommentFileSizeAuthor
#3 convert-block-content-to-render-array-1735244-3.patch4.59 KBdevin carlson
#1 convert-block-content-to-render-array-1735244-1.patch5.23 KBdevin carlson

Comments

devin carlson’s picture

devin carlson commented
Status: Active » Needs review
StatusFileSize
new convert-block-content-to-render-array-1735244-1.patch5.23 KB

Attached is a patch to convert the block content to a render array.

ZenDoodles’s picture

ZenDoodles commented
Status: Needs review » Reviewed & tested by the community

This looks great. Functionally it's good. I have a couple of tiny comments, but none of them really deal breakers, so I'm also marking rtbc. You can do the honors if you like.

+++ b/twitter-block-tweet.tpl.php
@@ -1,4 +1,4 @@
-<div class="twitter_block tweet">
+<div class="clearfix tweet">

Let's leave the twitter_block in here too in case someone has already used it in a theme? Also, isn't the clearfix part of another patch somewhere?

+++ b/twitter_block.module
@@ -220,7 +204,31 @@ function twitter_block_load_tweets($config) {
+  foreach($results as $tweet) {  ¶
+    $tweets[] = theme('twitter_block_tweet', array('tweet' => $tweet, 'api' => $twitter->getApi())); ¶

Trailing white-space

+++ b/twitter_block.module
@@ -220,7 +204,31 @@ function twitter_block_load_tweets($config) {
+  // Define the configuration.
+  $block = array();

I think maybe this comment is not accurate anymore?

+++ b/twitter_block.module
@@ -228,13 +236,13 @@ function twitter_block_load_tweets($config) {
-      'variables' => array('tweet' => NULL, 'api' => NULL),
+      'variables' => array(
+        'tweet' => NULL,
+        'api' => NULL,
+      ),

The style guide says to break it up if it extends past 80 chars. I know it's nit-picky, but IMO in this case the original is easier to read. Not a deal breaker tho. If it's important to you leave it.

devin carlson’s picture

devin carlson commented
StatusFileSize
new convert-block-content-to-render-array-1735244-3.patch4.59 KB

Rerolled to address the concerns outlined in #2.

  • Moved the twitter_block class out of the render array and back into the template file and kept the CSS file in the same location.
  • Removed the whitespace.
  • Removed the unnecessary comment.
  • Restored the theme variables to a single line. I do prefer associative arrays to list each key + value on a separate line but it doesn't bother me.
devin carlson’s picture

devin carlson commented
Status: Reviewed & tested by the community » Fixed

Committed to 7.x-1.x.

Status: Fixed » Closed (fixed)

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