BetterTip

Hi,

this looks like it could be a handy module,

but first all where is the module file, did you forget to push it to drupal?

If you're not to sure about the whole module development thing then go to:

http://drupal.org/developing/modules

Once you have your module file sorted you'll want to look at this link:
http://ventral.org/pareview/httpgitdrupalorgsandboxmonymirza1848424git

Here you'll find errors and warnings in code based on the current Drupal coding standards.

You'll want to create a version specific branch and remove master branch, change the Read Me.txt file to README.txt.

But at this moment there is no real code in terms of the drupal module architecture that functions to review.

This should help you fix this Moving from a master branch to a version branch.

The module is really small, but here is a little review. Using the following as a structure for my review http://drupal.org/node/894256 and http://groups.drupal.org/node/184389

Here is a review of the 2.x branch, is it right?

Manual review:

project page

You should add a little bit more information on how this module work. Maybe something like in the "Issue Summary", and even copy that to the README.txt.

Code too short

This project is too short to approve you as git vetted user. We are currently discussing how much code we need, but everything with less than 120 lines of code or less than 5 functions cannot be seriously reviewed. However, we can promote this single project manually to a full project for you.

bettertip.js
line 2: You should replace "Drupal.behaviors.exampleModule" with something like "Drupal.behaviors.bettertip".
lines 5 to 25: This could be indented to make it more obvious that it belongs to the attach (3 indent).

bettertip.css
line 1: The @charset is not needed I think (never saw it in other modules).
line 5: The background use an absolute url, but the module may be installed in a different location, for instance /profiles/a_super_distrib/modules/contrib/bettertip. I think it should be url("tool.png")

bettertip.info
Remove the "core" at line 11, it's already defined at line 4
At line 6 there is a configure path but this point to a non-existant page.

bettertip.module
The file is still empty, is that normal?

Could you add the information you give in the issue summary to your project page?

For the code length you should not add code just to have 120 lines. As mentioned the project can be promoted but you will not be approved as a git vetted user. You could add a configuration page but I don't know which part may be configured. (+ it will certainly not be enough to have 120 lines)

I see that you defined the minimum jquery version to 1.7 but I don't see any function in your js not available in 1.4. I may be missing something. Any reason for that?

Please note: I'm not 100% of what I'm saying. It's just my humble opinion...

If you require jQuery 1.7+ you should add the dependency of jQuery Update module (and as for now, only the development version of jQuery Update can update jQuery to 1.7).
As far as I know CSS class names should include the name of your module. Otherwise, classes named tool or tip will probably cause conflicts with existing markup.

README.txt: There are some encoding issues, please encode all files in utf-8. Please also make sure you use the single quote character '.

bettertip.js:

• Please follow all style guidelines. For example, put spaces after commas in $('.tool').html(alt1).show().css('padding','10px 30px');
• Why do you do css('padding','10px 30px') in

  $('*[title]').mouseenter(function() {
  var alt1 = $(this).children('span.tip').text();
  $('.tool').html(alt1).show().css('padding','10px 30px');
  });

  ?
  You already have that style in your css file.

• In $(this).append('<span class="tip" style="display: none;">' + alt + '</span>'); you have inline styles. Can't display: none; be in you tip class?
• I think you could use the jQuery selector [title] instead of *[title]
• The JavaScript code seems not very efficient. For example this:
  

  $(document).mouseover(function(e){
    var topp = e.pageX + 'px';
    var bott = e.pageY + 'px';
    $('.tool').css({'left': topp,'top': bott});
  });

  could be replaced with:

  var topp;
  var bott;
  var $tool = $('.tool');
  $(document).mouseover(function(e){
    topp = e.pageX;
    bott = e.pageY;
  });
  $('*[title]').mouseover(function(e){
    $tool.css({'left': topp + 'px', 'top': bott + 'px'});
  });
  

  EDIT: I don't know for sure, nor I've not done any benchmarks.

• Also, topp and bott vars have misleading names. Please choose better names.

Yes, I always forget that I should test things before :)
But the idea, which I think is still valid, was to not do a $('.tool').css({'left': topp,'top': bott}); in every mouse move.

Hello,

I am unable to clone the git repo with the command above.

I get :
fatal: http://drupalcode.org/sandbox/monymirza/1848424.git/info/refs not valid: is this a git repository?

I am able to clone with the command on the project page, can you adjust the command in the post above to so that it works as well?

A nice guide for creating a better product page can be found here: http://drupal.org/node/997024

I also noticed that you are creating some variables to store the configuration data. I believe you should create an install file for the module to remove those variables if the module is ever uninstalled. The hook you would want to look at is hook_uninstall.

Reviews of other projects

bettertip.module

Line 30: The module should define its own administer permission eg. 'administer bettertip' using hook_permission() rather than relying on 'access administration pages'.

You could do with some validation to ensure that valid hex values or CSS keywords are entered in the admin form. Users are likely to do obvious things like miss off the hash.

Line 64-70: Inline CSS should be added in hook_init(), not just in the root of the module file.

A better approach for the configurable colours might be to pass the values to the JavaScript via the settings object and adding them as inline styles rather than adding inline CSS.

I'm not a big fan of the idea of putting CSS values in an admin form anyway and would rather just leave it to the user to theme it, but that's just my opinion.

I think it's best practice to delete any variables you set in a hook_uninstall function.

bettertip.js

Code indenting is a bit all over the place.

Would be better to create the .bettertip element once and show/hide it rather than keep adding and removing it from the DOM.

Line 22: Caching a reference to $('.bettertip') would give better performance than looking it up on every mouse move event.

bettertip.css

Line 4/6: No need to define colours here as they're always overridden by your inline code.

Line 5: Might want to add vendor prefixed versions of border-radius. Also border-radius: 5px; works fine without the repeated values.

how it can be done?

You can do reference = $('.bettertip');:

(function ($) {
  Drupal.behaviors.bettertip = {
    attach: function (context, settings) {
     $bettertip = $('.bettertip');

     $('[title]').hover(function(e){
       var titleText = $(this).attr('title');
       if(titleText != ''){
       $(this).data('tiptext', titleText).removeAttr('title');
       }
       $('<p class="bettertip"></p>')
       .text(titleText)
       .appendTo('body')
       .css('top', (e.pageY - 10) + 'px')
       .css('left', (e.pageX + 20) + 'px')
       .fadeIn('slow');

       }, function(){
         $(this).attr('title', $(this).data('tiptext'));
         $bettertip.remove();
       }).mousemove(function(e){
         $bettertip.css('top', (e.pageY - 10) + 'px').css('left', (e.pageX + 20) + 'px');
       });

    }
  };
})(jQuery);

The problem with netol's solution is that you're doing the query before you've created the element.

Something like this would allow you to create the element once and reuse it:

(function ($) {
  var $bettertip;
  
  Drupal.behaviors.bettertip = {
    attach: function (context, settings) {
      if (!$bettertip) {
        $bettertip = $('<p class="bettertip"></p>');
        $bettertip.appendTo('body').hide();
      }
      
      $('[title]').once('bettertip-processed').each(function () {
        var $$ = $(this),
          titleText = $$.attr('title');

        $$.removeAttr('title');

        $$.hover(function (e) {
          $bettertip
            .text(titleText)
            .css('top', (e.pageY - 10) + 'px')
            .css('left', (e.pageX + 20) + 'px')
            .fadeIn('slow');
        }, function () {
          $('.bettertip').hide();
        });

        $$.mousemove(function (e) {
          $bettertip
            .css('top', (e.pageY - 10) + 'px')
            .css('left', (e.pageX + 20) + 'px');
        });
      });
    }
  };
})(jQuery);

Other things to note:

• Using .once() avoids adding the tooltip behavior more than once to the same element eg. if attach gets called again when AJAX content is loaded.
• There's nothing to stop the tooltip being positioned outside the viewport if the element is near the right hand edge of the browser. There are plenty of jQuery tooltip plugins that already handle this kind of thing. I'd suggest looking at them for reference or considering using one as a library rather than writing your own from scratch.
• The tooltip's position relative to the mouse and the show/hide effects used would be good to make configurable on the admin page. Much more so than the colours.

Nice simple module that works.

No show stoppers here.

Manual review
- I'd rename the bettertip_form to something like bettertip_admin_settings_form. There's a hook_form() that this isn't implementing and your function won't be called, it could be confusing.
- You should put "Implements hook_init()." above bettertip_init() as the comment. You should do this for any hook you are implementing so it's clear. Move the comment you have there inside the function.

manual review:

1. project page is a bit short, see http://drupal.org/node/997024
2. There is a wrongly named "7.x-1" branch in git which should be removed.
3. bettertip_init(): this is dangerous since the variables are user provided input and should be sanitized before printing to avoid XSS vulnerabilities. I could not exploit this because the input textfields are limited in size, but it is recommended to sanitize here. See also http://drupal.org/node/28984

Thanks for your contribution, monymirza!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here 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 lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, 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.

Thanks to the dedicated reviewer(s) as well.