[D7] Nano Scrollbar

• Please add [D7] to the title.
• Your repository link includes your username, the correct one is:
  git clone --branch master http://git.drupal.org/sandbox/kshyam.kumar1987/2219607.git nano_scrollbar
• You should delete the master branch and put your code in a version-specific branch, 7.x-1.x in this case.
• Delete LICENSE.txt and remove "version", "project", and "datestamp" from nano_scrollbar.info,they will be added by drupal.org packaging automatically.
• You state that there are detailed installation instructions for this module on d.o and in the README, but the d.o-page says "coming soon" and the README refers to d.o.
• In nano_scrollbar_page_alter(), remove $vert_css = $content['vertical_css']; because it is not used.
• nano_scrollbar.admin.inc does not conform to Drupals coding standard, according to PHP-CoderSniffer:
  

  --------------------------------------------------------------------------------
  FOUND 12 ERROR(S) AND 2 WARNING(S) AFFECTING 12 LINE(S)
  --------------------------------------------------------------------------------
    88 | WARNING | A comma should follow the last multiline array item. Found:
       |         | FALSE
    99 | ERROR   | Inline comments must start with a capital letter
   105 | ERROR   | Concatenating translatable strings is not allowed, use
       |         | placeholders instead and only one string literal
   107 | ERROR   | Inline comments must start with a capital letter
   107 | ERROR   | Inline comments must end in full-stops, exclamation marks, or
       |         | question marks
   114 | ERROR   | Concatenating translatable strings is not allowed, use
       |         | placeholders instead and only one string literal
   116 | ERROR   | Inline comments must start with a capital letter
   116 | ERROR   | Inline comments must end in full-stops, exclamation marks, or
       |         | question marks
   125 | ERROR   | Concatenating translatable strings is not allowed, use
       |         | placeholders instead and only one string literal
   136 | ERROR   | Inline comments must end in full-stops, exclamation marks, or
       |         | question marks
   143 | ERROR   | Concatenating translatable strings is not allowed, use
       |         | placeholders instead and only one string literal
   152 | ERROR   | Concatenating translatable strings is not allowed, use
       |         | placeholders instead and only one string literal
   161 | ERROR   | Concatenating translatable strings is not allowed, use
       |         | placeholders instead and only one string literal
   169 | WARNING | Avoid backslash escaping in translatable strings when
       |         | possible, use "" quotes instead
  

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxkshyamkumar1987221960...

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Simple nice module :)
I have only one note. As far as I know, js-coding standards still imply an indent of 2 spaces, not 4.
https://drupal.org/node/172169#indenting

Perhaps, pareview don't check js. Maybe I'm wrong.

There should be a link on Status Reports page to download the required nanoscrollbar library.
Also after applying settings the scroll bar get applied to whole node including comments, title etc. whole content got messed up.
Refer screenshot for the settings I used.

Hi Shyam,

Nice module - I think this is a really good looking scroll bar :)

Project Page

The project page should link to similar modules including Scrollbar and Neat Scrollbar and say how your module differs from these ones.

Useability Review

Installation instructions should be in a readme as well as on Drupal.org as it took me a while to find them.

Installation instructions read:

If you don't already have the overthrow plugin installed and wish to use it with this module, you should ensure that it is installed in the following folder:
sites/all/libraries/overthrowmin
The 'overthrow' library folder should contain the following file:

So not sure if the folder should be called overthrowmin or overthrow? I was also unsure what this extra library did or why I would want to use it. I think a short description of what it does would be beneficial.

I think the need for a trailing comma in the "CSS Attributes" field is not desirable and unnecessary. Also I wonder if the title jQuery selector may be more meaningful here as opposed to CSS Attributes?

Whilst this is going beyond the scope of this review I found that having to add so much custom CSS to my themes style sheet (to be able to use the custom CSS selector feature) seemed to take away from the usability of the module slightly. It seems the only information I actually had to supply was the height that I wanted my block to be. So, my idea was to edit the config page and instead of entering a comma separated list of selectors, have a multi-value field allowing used to enter a CSS selector and next to that a desired height. There would then be an add more button to allow used to enter another selector and another height. The module could then auto generate the CSS for each block removing the need to the users to edit their themes CSS. Obviously this is not a RTBC blocker but just an idea for the future.

The prevent page scrolling feature worked as expected however I didn't understand what the Disable Resize setting was supposed to do. Doesn't the plugin just add a scrollbar - what is getting resized? I did check the nanoScrollerJS documentation however it didn't make it any clearer. Could the field description be a little more descriptive here?

I didn't check the Enable iOSNativeScrolling feature as don't have an iOS device.

I enabled the Flash On/Off feature and then reloaded the page with my block on a few times but could not get the scroll bar to flash. I could see that Drupal.settings.nanoScrollbarSet.flash was set to 1 so not sure what the problem is here.

I also could not see any affect from enabling the Destroy On/Off setting. I was expecting the nano scroller to be replaced with the browsers scroll bar but the nano scroll bar remained. Again the Stop On/Off setting had no noticeable affect. I think these settings need to be documented better (assuming I just don't understand what they are meant to do), fixed (assuming they just don't work at the moment), or removed all together.

I don't have a mobile browser available so did not test this feature, however I wondered why anyone would want this feature off? Are there more files that need to be loaded when this is enabled? If this is the case I think the field description should mention this so users can be an informed choice about whether or not to use this feature.

Code Review

Could not find any problems.

Hi Shyam,

Its very Nice module - I think this is a really good looking scroll bar :)

Manual Review

Drupal.behaviors.nanoScrollbar.attach uses Drupal.settings directly, and not the settings that have been passed in.

Line 39 of the JS doesn't use the context that has been passed in.

nano_scrollbar_page_alter() should use #attached and not drupal_add_css() and drupal_add_js().

nano_scrollbar_preprocess_node() and nano_scrollbar_preprocess_block() have variable_get() without a default value.

In nano_scrollbar_preprocess_block(), using an explicit index for classes_array is fragile. You need to figure out a better test.

Your CHANGELOG.txt references explicit version numbers that don't exist.

_nano_scrollbar_flashdelay_validate() isn't needed; you can use element_validate_integer_positive() instead.

Add links and info for installation to the project page. People don't read READMEs...

Add a link to overthrowmin in the README

Your project page should mention any similar scrollbar modules, even if they don't use nanoscrollbar.

Looks like a nice module addition once you polish this a bit.

I'm putting this back to Needs Review, because I shouldn't have put this back to Needs Work as (I now know) none of the issues I mentioned are blockers. Someone should take a look at it though for any recent work.

Links. Most things are in the Handbook, or in the API docs. Some of the comments are based on experience. The best JS ref is https://drupal.org/node/756722

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

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

  FILE: /home/klausi/pareview_temp/js/nano_scrollbar_default.js
  --------------------------------------------------------------------------------
  FOUND 4 ERRORS AFFECTING 2 LINES
  --------------------------------------------------------------------------------
    1 | ERROR | Missing file doc comment
   43 | ERROR | No space before comment text; expected "// end" but found "//end"
   43 | ERROR | Inline comments must start with a capital letter
   43 | ERROR | Inline comments must end in full-stops, exclamation marks, or
      |       | question marks
  --------------------------------------------------------------------------------
  
  FILE: /home/klausi/pareview_temp/nano_scrollbar.module
  --------------------------------------------------------------------------------
  FOUND 2 ERRORS AFFECTING 2 LINES
  --------------------------------------------------------------------------------
   110 | ERROR | [x] Whitespace found at end of line
   128 | ERROR | [x] Whitespace found at end of line
  --------------------------------------------------------------------------------
  PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  --------------------------------------------------------------------------------
  

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. Some of mpdonadio's comments still need fixing, everything he reported should be fixed.
2. nano_scrollbar_page_alter(): why do you need hook_page_alter()? You could just add your JS/CSS in nano_scrollbar_preprocess_node() and nano_scrollbar_preprocess_block(), that would be enough? What other reason would there be that you need it on every single page request? Please add a comment.

But otherwise looks good to me. Since this was RTBC already and mpdonadio only found issues that are not absolute critical application blockers we can approve this.

Thanks for your contribution, shyam kumar kunkala!

I updated your account so you can 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 stay 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.