Article Jump

Bartlantz - It's very handy module and code is almost perfect.

.module file

There is one issue in hook_init(). You haven't added mousetrap.min.js in your module file but in the else condition, you are referring to it.

if ($path = libraries_get_path("mousetrap")) {
  drupal_add_js($path . '/mousetrap.min.js');
}
else {
  drupal_add_js(drupal_get_path('module', 'article_jump') . '/mousetrap.min.js');
}

So either you may like to remove this else condition or you may want to add mousetrap.min.js in your module folder.

READEME.txt file

In Installation, point 3, you have mentioned incorrect folder name 'library'. It should be 'libraries'.

@bartlantz,

Very nice module, it is simple, direct & most of all just works!

Here are some issues / suggestions I've found digging into your code.

Architectual Design Issues:

• Article Jump does not considre user manual scroll:
  Say I'm using your module to scroll down some articles than I use the scrollers to go up the last article clicking n will bring me down twice and clicking p will leave me where I am.
  This can be fixed by checking the scroll position when the user clicks n & p.
  Alternatively you could offer a choice for the admin to select.

Suggestions:

• Switching n&p to j&k:
  Though I understand that n stands for Next and that p stands for Previous, You should remember that usually only power users use keyboard shortcuts and they are used to using j & k.
• Using self executing functions (JavaScript):
  While it isn't in the JavaScript coding standards it is good practice to wrap your javascript code inside a self executing function.
  Using the above will replace your need of using the jQuery keyword instead of the common $ sign.
  You can learn more about self executing functions in here.
• Alerting when the library is not installed:
  It can be useful for site developers if an error will show (in the module settings page) when the moustrap library is not installed.
• Removing the install function:
  I do see why the uninstall function is usefull there is no need to add the install function. These values are defined in the second argument of variable_get (the default value argument).
• Declare JavaScript in info file:
  In Drupal 6 & 7 we can declare javascript file stright from the info file using the scripts[] array. You can read more about it here.

added reference to demo site

Bart,

I'm flattered you accepted my suggestions.

P.S.
About loading the article_jump.js using the info file, because you are using the behaviors attach function I don't think you'll have a problem to use mouse trap. When I tested it it worked fine. Even if the mouse trap file is loaded after your own javascript file the attach function happens much later.

Regards,
Eyal

Review of the 7.x-1.x branch:

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. article_jump_permission(): that permission is never used?

But that are just minor issues, otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

hotkeys

What about the permission "administer article_jump" that is never used?

No objections in more than a week, so ...

Thanks for your contribution, bartlantz!

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.

Congratulations