jQuery Scroll Follow

hi,

did manual review of your code.
Found following issues:-
1. You have followed proper indentation in your init function. Please refer http://drupal.org/coding-standards/#indenting for this purpose. Here you will get better idea how it should be done.
2. you have used variable_get function and not set its default value which is incorrect practice. You must set its default value whenever you call this function.
3. Make sure every file in your module ends with an new line, which is not happening currently.
4. You havent mentioned core in your info file, which i believe should always be there.

For automated review click here. You are also supposed to fix these issues.
Thanks

You are using module Libraries API to get external .js library, but you have no dependency in your .info file. Please, add proper dependency.

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732

Some typos in settings form - of course it is not a bug.
Css classe(s) - should be CSS class(es) probably or simply CSS classes
Css classes (separated by a comma) - CSS classes (separated by a comma)

I think that if you want to have classes separated by a comma there is no need for textarea field. Simple textfield should be OK.

Your module has it's own settings page, so it would be good to put path to it in .info file
configure = path/to/your/settings
Small icon will appear on module list then.

Also you do not need to type what value is a default value.
Even if user just installed your module variable_get function will show default value right away.
If you want users to have a chance to restore default values I would suggest adding "Restore default" button in settings page.

I was able to install module without Libraries API (dependency problem) and use it without js file in libraries folder.
It makes my all other scripts not working because of fatal error. You should probably check if there is a file and if there is no file do not add it to the code. It is not sufficient to place a warning in status file.
That's the only one reason I put status to needs work. Of course other issues (master branch) should also be fixed.

What's more in status report there is something wrong with the message.

Great module!

Stumbled too on the js fatal error. Patch attached.

Hi there, useful module, could of done with this a few days ago.

Couple of minor points..

The plugin can also be passed a CSS ID instead of a class, also maybe explain that the DOT is required when specifying css classes, as I left them out originally when testing the module, maybe the description would be better worded as 'CSS selectors (Separated by a comma).

Line 33: the description should specify the Units e.g.
'The duration of the sliding animation (in milliseconds). The smaller the value, the faster the animation. (Default:500)'

Line 40: the description could be clearer e.g.
'Number of pixels the Scroll Follow object should remain from top of viewport. (Default:0)'

Line 47: the delay description could be clearer e.g.
'Time between the end of the scroll and the beginning of the animation in milliseconds. (Default:0)'

Lastly and this is NOT an issue, more of a question.... A lot of jQuery libraries i have installed use the path sites/all/libraries/jquery.PLUGINNAME/PLUGINNAME.js i'm not sure if there is any guidance on this at all but would it be cleaner if the default library path was sites/all/libraries/jquery.scrollfollow/jquery.scrollfollow.js ?

Hi !

I've been looking around for some time as the library was not loading I guess it was a path problem and I succeed to solve it ! The problem is inside jquery_scroll_follow_init(). Line 68 of the .module.

1. Add global $base_path; at the begining.

2. Add this on the file_exists check.
if (file_exists($_SERVER['DOCUMENT_ROOT'] . $base_path . $jquery_scroll_follow_location . '/jquery.scrollfollow.js')) {

Now the module works :-)

I don't see any major problem on it.

file_exists() function doesn't take relative paths.
See: http://www.php.net/manual/en/function.file-exists.php

Not all installations are on the public_html/ path of the server, sometimes Drupal can be in a subfolder.
In my case ( working with Ubuntu lamp ) I install it inside /var/www/d7/ and as I have not setup the Hosts for this Drupal site, I access the site with the following URL: http://localhost/d7/

In this particular situation $base_path will contain "d7/".
If the website was identified with a DNS pointing to that sub folder, for example http://local.d7/, the $base_path would contain "/".

File_exists tries to find the file in the internal server structure context.
This is why it needs the full internal path, not the URL path.

To make the file_exists function work correctly you need to give him the full internal path of the file.
In my case it would be:

"/var/www/d7/sites/libraries/jquery_scroll_follows/"

That path can be dynamically build using:

$_SERVER['DOCUMENT_ROOT'] . $base_path . $jquery_scroll_follow_location

You can also use the php function is_file() instead of file_exists(), as it is x2 faster.
See: http://be.php.net/manual/en/function.is-file.php

jquery_scroll_follow.js and jquery_scroll_follow_start.js appear to be identical, but only jquery_scroll_follow_start.js is referenced with drupal_add_js();

jquery_scroll_follow_start.js needs a little formatting TLC.

The README.txt mentions Libraries as a dependency, but it is not mentioned in the .info file as one.

The current library archive, jquery-scroll-follow.zip, has the JS file in a lib/ subdirectory. You should either use this location, or be really explicit in the docs what the path should be.

jquery_scroll_follow_requirements() has a small logic error in line 41. Your outer case is limiting this to the runtime phase, so this will always evaluate to REQUIREMENT_ERROR;

Long term, you may want to consider a better way to expose the feature instead of using a hook_init(). Not sure what you can do here, but there has been a trend to move away from using hook_init() and to use more specific hooks so that page request weight is lessened.

You may also want to add some logic to make sure the settings are "sane" before adding to the page. For example, make sure the CSS selector is valid, the other settings are numbers, etc.

Personally, I would have an "all OK" message on the status report and not just an error in your hook_requirements().

I installed and configured everything, set the settings (used .node as the selector), saw the JS on the page, but couldn't get anything to work.

May want to consider explicitly passing in Drupal and the JS settings, which prevents problems if another file alters them by accident:

(function ($, Drupal, settings, undefined) {
  // ...
})(jQuery, Drupal, Drupal.settings.jquery_scroll_follow);

You may also want to do a parseInt(foo, 10) in case people use leading zeros by accident.

The change to hook_requirements(), you still have something that looks like

switch ($phase) {
  case 'runtime':
    if (...) {
    } else if ($phase == 'runtime') {
    }
}

Is that what you were going for?

Not working. When I scroll around, I can watch the changes with Firebug / FireQuery, but nothing happens. I suspect I need some CSS changes to handle the positioning added to my theme. My sandbox was just using the default Bartik. I suspect you just need a little more documentation around this.

Manual review: nice simple plugin and code was clean.

(not an expert drupal reviewer so learning as I review )

I think this looks fine now. It is unclear to me, though, whether I can mark this RTBC or whether @klausi has to do that to move things to the next step.

As I understood from the explanations of Klausi during last Drupalcon ( http://munich2012.drupal.org/program/sessions/project-application-review-process - min 17 ) you can set it as RTBC if you are confident about the code quality.

manual review:
"'access arguments' => array('access administration pages'),": The administration menu callback should probably use "administer site configuration" - which implies the user can change something - rather than "access administration pages" which is about viewing but not changing configurations.

As this is a minor security issue I have to put this back to needs work, otherwise all looks good. Set this back to RTBC directly after you fixed the permission.

Thanks for your contribution, Jibus!

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.