BeautyTips Form Errors

Automated review:
http://ventral.org/pareview/httpgitdrupalorgsandboxartemtaranyuk1925868git
- found some errors

Manual review:
- Readme.txt is missing
- move from master to a branch i.e. 7.x-1.x
- Module is a nice idea, code looking good and well commented, i think when automated reviews will pass, the project is good prepared

- Yes i think that's ok with the 2 warnings (for the too long comment-lines)
- It looks better if the review-scripts run through without any issues (only a suggestion)
- You can also try the Coder module to assure there are no more coding standard issues.
- You now can delete the master branch.

This sounds like a feature that should live in the existing beautytips project. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the beautytips issue queue to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward. If you cannot reach the maintainer(s) please follow the abandoned project process.

If that fails for whatever reason please get back to us and set this back to "needs review".

The module works really nice, but from admin/config/user-interface/beautytips I saw no way to get to admin/config/user-interface/beautytips/bt-form-errors. Did I miss something? I had to go into hook_menu to find the admin link.

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and put yourself on the PAReview: review bonus high priority list. Then I'll take a look at your project right away :-)

Git repository link fixed.

I have reviewed this module and it seems to be in good condition. The code and comments are well-formed according to Drupal's standards. The docblock comments are sufficiently extensive and explanatory. The code is well-organized in appropriately named functions and divided between code in the .module file and administrative code in the .admin.inc file. I only found a couple of minor code issues listed below:

bt_form_errors.module line #117

return array(
  'bt_form_errors_messages' => array(
    'arguments' => array('display' => NULL),
  ),
);

This code in the module's implementation of hook_theme() uses the key 'arguments' when it should be 'variables'. 'arguments' was the key name in the Drupal 6 version of hook_theme().

bt_form_errors.module line #304
$settigns = bt_form_errors_get_settings();

This variable is misspelled here and throughout the rest of the function definition.

bt_form_errors.js

$('input.error, textarea.error, select.error', context).each(function(){
  $(this).bt($(this).attr('data-error-text'), Drupal.settings.bt_form_errors);
  $(this).btOn();
});

You should cache $(this) to improve performance (e.g. var currentElement = $(this);). Then jQuery will only have to turn "this" into a jQuery object once instead of three times.

Also, for the future I suggest that you consider creating some SimpleTest tests so that your module can be automatically tested. See http://drupal.org/simpletest for more info.

Hi artem.taranyuk,

Everything looks fine. The only thing what isn't very practical is what is mentioned in #8, you can't reach the settings page if you don't have admin_menu installed.
I also think it would be better of in the beautytips module, but I do understand that you don't want to maintain somebody else's code.

Gr,
Samuel

manual review:

1. "define('BFE_VISIBILITY_NOTLISTED', 0);": all constants defined by your module need to be prefixed with your module's name to avoid name clashes.
2. "variable_set('bt_form_errors', $settings);": all variables defined by your module need to be removed in hook_uninstall().
3. bt_form_errors_theme(): variables for the theme function are missing?

Although you should definitively fix those issues I think they are not absolute application blockers, so I guess this is RTBC. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to chx as he might have time to take a final look at this.

Added one more review.

Hello artem.taranyuk:
Use canvas for background is wonderful ideal. But I find a problem after install.If the form is left side,the the form error will block the content right.Please see the picture attached .

I don't think the comment from jeffstric is a blocker and there were no other objections for more than a week, so ...

Thanks for your contribution, artem.taranyuk!

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.