willsteinmetz [willsteinmetz]

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review your code, pointing out what needs to be changed.

As per http://drupal.org/cvs-application/requirements, the motivation message should be expanded to contain more details about the features of the proposed module, and it should include also a comparison with the existing solutions.

I don't necessarily think you should be using the FB Drupal module, but you have not mentioned this at all. Can you describe the difference between your module and the FB module. http://drupal.org/project/fb

Some thoughts:

1. All code files should have @file docblocks.

2.     // Make sure the user has access to use TweetMeme.
   

   :)

3. You are not deleting the "facebookshare_text" variable in the uninstall hook.

4.   $form['facebookshare_sizes_list'] = array(
       '#type' => 'markup',
       '#value' => '<strong>Share button types:</strong><div style="clear: both;">box_count: ' .
         '<a name="fb_share" type="box_count"' .
         'share_url="http://www.facebook.com/facebook-widgets/share.php"></a>' .
         '<script src="http://static.ak.fbcdn.net/connect.php/js/FB.Share" ' .
         'type="text/javascript"></script></div>' .
         '<div style="clear: both;">button_count: <a name="fb_share" type="button_count"' .
         'share_url="http://www.facebook.com/facebook-widgets/share.php"></a>' .
         '<script src="http://static.ak.fbcdn.net/connect.php/js/FB.Share" ' .
         'type="text/javascript"></script></div>' .
         '<div style="clear: both;">button: <a name="fb_share" type="button"' .
         'share_url="http://www.facebook.com/facebook-widgets/share.php"></a>' .
         '<script src="http://static.ak.fbcdn.net/connect.php/js/FB.Share" ' .
         'type="text/javascript"></script></div>' .
         '<div style="clear: both;">icon_link: <a name="fb_share" type="icon_link"' .
         'share_url="http://www.facebook.com/facebook-widgets/share.php"></a>' .
         '<script src="http://static.ak.fbcdn.net/connect.php/js/FB.Share" ' .
         'type="text/javascript"></script></div>' .
         '<div style="clear: both;">icon: <a name="fb_share" type="icon"' .
         'share_url="http://www.facebook.com/facebook-widgets/share.php"></a>' .
         '<script src="http://static.ak.fbcdn.net/connect.php/js/FB.Share" ' .
         'type="text/javascript"></script></div>',
     );
   

   You should figure out a way to utilize your theme function to display these as someone could override the theme and this would no longer be representative of the buttons. Also, having all that output not in a theme is just kind of icky.

5. <div style="float: right; clear: right;">
   

   You should be using CSS for this. This is very difficult, if not impossible to override.

6.     // Check in the teaser view.
       if ($teaser && !empty($location['teasers'])) {
         $node->content['facebookshare'] = array(
           '#value' => theme('facebookshare', $url),
           '#weight' => -10,
         );
       }
       // Check in the full view.
       else if(!$teaser && !empty($location['content'])) {
         $node->content['facebookshare'] = array(
           '#value' => theme('facebookshare', $url),
           '#weight' => -10,
         );
       }
   

   Could be better written as:

   
       // Check what view and if allowed
       if (($teaser && !empty($location['teasers'])) || (!$teaser && !empty($location['content']))) {
         $node->content['facebookshare'] = array(
           '#value' => theme('facebookshare', $url),
           '#weight' => -10,
         );
       }
   

Overall, pretty minor changes. This is real close. Good work.

Getting there:

1. /**
    * Implements hook_init()
    */
   function facebookshare_init() {
     drupal_add_css(drupal_get_path('module', 'facebookshare') . '/facebookshare.css');
   }
   

   You should not be putting CSS in an init hook unless it really needs to be on every page. You should add your CSS before you call your theme function in the nodeapi hook.

2.   $form['facebookshare_sizes_list'] = array(
       '#type' => 'markup',
       '#value' => '<strong>Share button types:</strong><div style="clear: both;">box_count: ' .
         theme(
           'facebookshare_button',
           'http://www.facebook.com/facebook-widgets/share.php',
           'box_count',
           t('Share')
         ) .
         '</div>' .
         '<div style="clear: both;">button_count: ' .
         theme(
           'facebookshare_button',
           'http://www.facebook.com/facebook-widgets/share.php',
           'button_count',
           t('Share')
         ) .
         '</div>' .
         '<div style="clear: both;">button: ' .
         theme(
           'facebookshare_button',
           'http://www.facebook.com/facebook-widgets/share.php',
           'button',
           t('Share')
         ) .
         '</div>' .
         '<div style="clear: both;">icon_link: ' .
         theme(
           'facebookshare_button',
           'http://www.facebook.com/facebook-widgets/share.php',
           'icon_link',
           t('Share')
         ) .
         '</div>' .
         '<div style="clear: both;">icon: ' .
         theme(
           'facebookshare_button',
           'http://www.facebook.com/facebook-widgets/share.php',
           'icon',
           t('Share')
         ) .
         '</div>',
     );
   

   You are not using the t() when needed. Also, I was hoping you would use a loop or something here. You could define a short array then loop through it and save lots of code and effort (not a huge deal). To really be up to standard, you should create a theme function for this, and all you would have to do is send it your array of types.

3.   $form['facebookshare_size'] = array(
       '#type' => 'select',
       '#required' => TRUE,
       '#options' => array(
         'box_count' => 'box_count',
         'button_count' => 'button_count',
         'button' => 'button',
         'icon_link' => 'icon_link',
         'icon' => 'icon'
       ),
       '#default_value' => variable_get('facebookshare_size', ''),
     );
   

   You don't have a title. Not necessary, but a suggestion would be to actually display each example next to the selection (this would require using a radio list).

4. Your CSS files needs a file docblock and $Id$

So close!

Your updated manifesto for the module is much better and totally acceptable.

1. .form-radios .form-item {
   	width: 130px;
   	float: left;
   }
   

   This CSS declaration will alter how all radios will look. You should use the element ID that gets put on the wrapper of the rendered element; using Firebug will make it really easy to see the HTML. Technically you should have a separate CSS file for your admin page since it is only needed on one page, but I don't think thats necessary here.

2. I think your CSS file is using tabs instead of spaces.

3.   // required in order to shift next form item below properly
     $form['spacer'] = array(
       '#type' => 'markup',
       '#value' => '<div class="facebookshare-spacer">&nbsp;</div>',
     );
   

   You should be able to target just that element and use the clear you need. You could also (though not suggested) use a "clear-block" class on the "facebookshare_size" element with attributes which should clear it out.

So close. The first one is important while the second two are not as necessary. I bet next one around will be it.

That's one good looking module! I would say this is ready to go. Good work.

Thank you for your contribution! I am going to update your account.
These are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, also, for your patience with the review process.
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

I thank all the dedicated reviewers as well.