Provides a dynamically-generated drop-down menu of the most important places on the page. This is an enhanced version of the widely-known "skipnav" mechanism for keyboard and screen reader users.

Usage

After installing, visit the Admin page to configure beyond the sensible defaults.
D7: admin/config/user-interface/skipTo

Sandbox Project Page

https://drupal.org/sandbox/ronfeathers/2017053

Git Repository

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/ronfeathers/2017053.git skipto

Reviews of other projects

https://drupal.org/node/1946966#comment-7537779
https://drupal.org/node/1989736#comment-7537831
https://drupal.org/node/2013591#comment-7537909
- - -
https://drupal.org/node/2021603#comment-7548973
https://drupal.org/node/2020219#comment-7552391
https://drupal.org/node/2018599#comment-7552439
- - -
https://drupal.org/node/2022569#comment-7561605
https://drupal.org/node/2023753#comment-7561621
https://drupal.org/node/2023073#comment-7561721

Comments

PA robot’s picture

PA robot commented
Status: Needs review » Needs work

Link to the project page and git clone command are missing in the issue summary, please add them.

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.

ronfeathers’s picture

ronfeathers commented
Status: Needs work » Needs review

Added fixes from automated code review

dclavain’s picture

dclavain commented
Status: Needs review » Needs work

Hi @ronfeathers:

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
Review of the 7.x-1.x branch:

  • Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
  • DrupalPractice has found some issues with your code, but could be false positives.
    
FILE: /var/www/drupal-7-pareview/pareview_temp/skipTo.module
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 4 WARNING(S) AFFECTING 4 LINE(S)
--------------------------------------------------------------------------------
  63 | WARNING | All variables defined by your module must be prefixed with
     |         | your module's name to avoid name collisions with others.
     |         | Expected skipTo_headings_array but found headings_array
  81 | WARNING | All variables defined by your module must be prefixed with
     |         | your module's name to avoid name collisions with others.
     |         | Expected skipTo_landmarks_array but found landmarks_array
 106 | WARNING | All variables defined by your module must be prefixed with
     |         | your module's name to avoid name collisions with others.
     |         | Expected skipTo_accessKey but found accessKey
 113 | WARNING | All variables defined by your module must be prefixed with
     |         | your module's name to avoid name collisions with others.
     |         | Expected skipTo_wrap but found wrap
--------------------------------------------------------------------------------

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.


FILE: /var/www/drupal-7-pareview/pareview_temp/js/SkipTo.min.js
--------------------------------------------------------------------------------
FOUND 217 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "!="; 0 found
 1 | ERROR | Expected 1 space after "!="; 0 found
 1 | ERROR | Inline control structures are not allowed
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "+"; 0 found
 1 | ERROR | Expected 1 space after "+"; 0 found
 1 | ERROR | Expected 1 space before "+"; 0 found
 1 | ERROR | Expected 1 space after "+"; 0 found
 1 | ERROR | Expected 1 space before "+"; 0 found
 1 | ERROR | Expected 1 space after "+"; 0 found
 1 | ERROR | Expected 1 space before "+"; 0 found
 1 | ERROR | Expected 1 space after "+"; 0 found
 1 | ERROR | Expected 1 space before "+="; 0 found
 1 | ERROR | Expected 1 space after "+="; 0 found
 1 | ERROR | Expected 1 space before "+="; 0 found
 1 | ERROR | Expected 1 space after "+="; 0 found
 1 | ERROR | Expected 1 space before "+="; 0 found
 1 | ERROR | Expected 1 space after "+="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Inline control structures are not allowed
 1 | ERROR | Expected 1 space after first semicolon of FOR loop; 0 found
 1 | ERROR | Expected 1 space after second semicolon of FOR loop; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before ">"; 0 found
 1 | ERROR | Expected 1 space after ">"; 0 found
 1 | ERROR | Expected 1 space before "+="; 0 found
 1 | ERROR | Expected 1 space after "+="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "+"; 0 found
 1 | ERROR | Expected 1 space after "+"; 0 found
 1 | ERROR | Expected 1 space before "+"; 0 found
 1 | ERROR | Expected 1 space after "+"; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "+="; 0 found
 1 | ERROR | Expected 1 space after "+="; 0 found
 1 | ERROR | Expected 1 space before "+="; 0 found
 1 | ERROR | Expected 1 space after "+="; 0 found
 1 | ERROR | Expected 1 space before "+="; 0 found
 1 | ERROR | Expected 1 space after "+="; 0 found
 1 | ERROR | Expected 1 space before "+="; 0 found
 1 | ERROR | Expected 1 space after "+="; 0 found
 1 | ERROR | Expected 1 space before "+="; 0 found
 1 | ERROR | Expected 1 space after "+="; 0 found
 1 | ERROR | Expected 1 space before "+="; 0 found
 1 | ERROR | Expected 1 space after "+="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Inline control structures are not allowed
 1 | ERROR | Expected 1 space after first semicolon of FOR loop; 0 found
 1 | ERROR | Expected 1 space after second semicolon of FOR loop; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before ">"; 0 found
 1 | ERROR | Expected 1 space after ">"; 0 found
 1 | ERROR | Expected 1 space before "+="; 0 found
 1 | ERROR | Expected 1 space after "+="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "+"; 0 found
 1 | ERROR | Expected 1 space after "+"; 0 found
 1 | ERROR | Expected 1 space before "*"; 0 found
 1 | ERROR | Expected 1 space after "*"; 0 found
 1 | ERROR | Expected 1 space before "+"; 0 found
 1 | ERROR | Expected 1 space after "+"; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "==="; 0 found
 1 | ERROR | Expected 1 space after "==="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "+="; 0 found
 1 | ERROR | Expected 1 space after "+="; 0 found
 1 | ERROR | Expected 1 space before "+="; 0 found
 1 | ERROR | Expected 1 space after "+="; 0 found
 1 | ERROR | Expected 1 space before "+="; 0 found
 1 | ERROR | Expected 1 space after "+="; 0 found
 1 | ERROR | Expected 1 space before "+"; 0 found
 1 | ERROR | Expected 1 space after "+"; 0 found
 1 | ERROR | Expected 1 space before "+="; 0 found
 1 | ERROR | Expected 1 space after "+="; 0 found
 1 | ERROR | Expected 1 space before "+="; 0 found
 1 | ERROR | Expected 1 space after "+="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "!="; 0 found
 1 | ERROR | Expected 1 space after "!="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Blank lines are not allowed after CASE statements
 1 | ERROR | Expected 1 space before "+="; 0 found
 1 | ERROR | Expected 1 space after "+="; 0 found
 1 | ERROR | Case breaking statements must be followed by a single blank line
 1 | ERROR | Blank lines are not allowed after CASE statements
 1 | ERROR | Expected 1 space before "-="; 0 found
 1 | ERROR | Expected 1 space after "-="; 0 found
 1 | ERROR | Case breaking statements must be followed by a single blank line
 1 | ERROR | Blank lines are not allowed after CASE statements
 1 | ERROR | Inline control structures are not allowed
 1 | ERROR | Case breaking statements must be followed by a single blank line
 1 | ERROR | Expected 1 space before ">"; 0 found
 1 | ERROR | Expected 1 space after ">"; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "==="; 0 found
 1 | ERROR | Expected 1 space after "==="; 0 found
 1 | ERROR | Expected 1 space before "-"; 0 found
 1 | ERROR | Expected 1 space after "-"; 0 found
 1 | ERROR | Expected 1 space before "==="; 0 found
 1 | ERROR | Expected 1 space after "==="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "==="; 0 found
 1 | ERROR | Expected 1 space after "==="; 0 found
 1 | ERROR | Expected 1 space before "-"; 0 found
 1 | ERROR | Expected 1 space after "-"; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Inline control structures are not allowed
 1 | ERROR | Expected 1 space after first semicolon of FOR loop; 0 found
 1 | ERROR | Expected 1 space after second semicolon of FOR loop; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before ">"; 0 found
 1 | ERROR | Expected 1 space after ">"; 0 found
 1 | ERROR | Expected 1 space before "+="; 0 found
 1 | ERROR | Expected 1 space after "+="; 0 found
 1 | ERROR | Inline control structures are not allowed
 1 | ERROR | Expected 1 space after first semicolon of FOR loop; 0 found
 1 | ERROR | Expected 1 space after second semicolon of FOR loop; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 1 | ERROR | Expected 1 space before ">"; 0 found
 1 | ERROR | Expected 1 space after ">"; 0 found
 1 | ERROR | Expected 1 space before "+="; 0 found
 1 | ERROR | Expected 1 space after "+="; 0 found
 1 | ERROR | Expected 1 space before "="; 0 found
 1 | ERROR | Expected 1 space after "="; 0 found
 4 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

Source: http://ventral.org/pareview - PAReview.sh online service

Manual review

  • Please specify in your project description the following data:
    1. A link to your project page.
    2. A direct link to your git repository (git clone ...).

    More information at https://drupal.org/node/1011698

  • Rename skipto.info To skipTo.info to be able to install the module.
  • Use the hook_uninstall() to delete the variables you use in the module.
  • I have installed your module and when I go to x get me the following warning:
        Warning: Invalid argument supplied for foreach() in form_type_checkboxes_value() (line 2288 of /home/dclavain/drupal-custom-7/includes/form.inc).
    Warning: Invalid argument supplied for foreach() in form_type_checkboxes_value() (line 2288 of /home/dclavain/drupal-custom-7/includes/form.inc).
  • Do you use the skipTo.php file?
ronfeathers’s picture

ronfeathers commented
Status: Needs work » Needs review

THANK YOU @dclavain !!!

issues addressed:
Passes PAReview for the most part now - regex in JS is failing, but to fix for PAReview breaks for js... not sure what to do there, will research.

Added links to project page as well as git URI

uses hook_uninstall to del variables now

skipTo.php removed

Thank you for your advice - this is my first module! :)

~R~

mpnkhan’s picture

mpnkhan commented

Looks like PAReview is reporting false positive for the regular expression. The regular expression is correct and it works..

ronfeathers’s picture

ronfeathers commented
Issue summary: View changes

Adding required info to description

ronfeathers’s picture

ronfeathers commented
Issue summary: View changes

add link to admin under usage

ronfeathers’s picture

ronfeathers commented
Issue summary: View changes

adds review link

ronfeathers’s picture

ronfeathers commented
Issue summary: View changes

another review comment

ronfeathers’s picture

ronfeathers commented
Issue summary: View changes

adds review comment

ronfeathers’s picture

ronfeathers commented
Issue tags: +PAreview: review bonus

adds "PAReview: review bonus" tag

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Needs work
Issue tags: -PAreview: review bonus

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

  • Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
  • DrupalPractice has found some issues with your code, but could be false positives.
    
FILE: /home/klausi/pareview_temp/skipTo.module
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 14 WARNING(S) AFFECTING 14 LINE(S)
--------------------------------------------------------------------------------
  48 | WARNING | 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.
  63 | WARNING | All variables defined by your module must be prefixed with
     |         | your module's name to avoid name collisions with others.
     |         | Expected start with "skipTo" but found "headings_array"
  81 | WARNING | All variables defined by your module must be prefixed with
     |         | your module's name to avoid name collisions with others.
     |         | Expected start with "skipTo" but found "landmarks_array"
 106 | WARNING | All variables defined by your module must be prefixed with
     |         | your module's name to avoid name collisions with others.
     |         | Expected start with "skipTo" but found "accessKey"
 113 | WARNING | All variables defined by your module must be prefixed with
     |         | your module's name to avoid name collisions with others.
     |         | Expected start with "skipTo" but found "wrap"
--------------------------------------------------------------------------------

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. "drupal_set_message("SkipTo Menu Settings updated:"": all user facing text must run through t() for translation. And please use placeholder with t() for the dynamic variables.
  2. "skipTo": module names should be all lower case.
  3. skipTo.js: appears to be 3rd party code. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms. The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

jamiehollern’s picture

jamiehollern
he/him
Primary language English
Location Alba/Scotland
commented

Just a few points on this:

  • Is there a reason that you put the JS in the footer as opposed to the header?
      drupal_add_js(drupal_get_path('module', 'skipTo') . '/js/skipTo.js', array('type' => 'file', 'scope' => 'footer'));
  • The JS file adds lots of inline styles to the elements it creates. I would recommend these styles be moved to a module CSS file that would allow a user to remove or override the styles they wish easily.
  • I realise that this is for screen readers and keyboard users, but when the menu is clicked it disappears. Is this intentional behaviour?
  • Also, this may not be an issue with your module, but I ran it on a fresh install and it wouldn't work until I'd cleared the Drupal caches. It may be that the theme registry needs to be cleared on enabling of your module.
ronfeathers’s picture

ronfeathers commented
Status: Needs work » Needs review

Buch of edits/updates here:

  • Text all goes through t()
  • variables prepended with module name
  • lowercased skipto (from skipTo)
  • skipto JS file externalized - now requires Libraries
  • rebuild registry on install now

@jamiehollern - thank you for your questions. The JS is in the footer because we don't want to block page download, and since we're not relying on jQuery we want the dom loaded before our init() fires.
Also, the CSS in inline to save an additional httprequest. There are build instructions on our GitHub
Page (https://github.com/paypal/SkipTo) that explains how to rebuild the file to suit your needs.

Thanks for looking!
Ron

ronfeathers’s picture

ronfeathers commented
Issue summary: View changes

can not add tag?

ronfeathers’s picture

ronfeathers commented
Issue tags: +PAreview: review bonus

re-adds PAReview: review bonus tag :)

chrisfree’s picture

chrisfree commented
Status: Needs review » Needs work

Ron,

I took some time to review this module this afternoon. Overall, I think it works well and is a nice little feature. Here are my notes and some things to consider.

  • Consider adding the configuration path to the README.txt file
  • I was confused when looking for the required JS file found here: http://paypal.github.io/SkipTo/ I looked for it at src/js instead of src/drupal/js - might be worth noting in the readme.
  • Following codes seems redundant
    <?php
  $skipto_jspath = libraries_get_path('skipto');
  if (module_exists('libraries') && function_exists('libraries_get_path')) {
    $skipto_jspath = libraries_get_path('skipto');
  }
?>

    I believe the first instance of libraries_get_path() should be removed?

  • Consider moving config form functions to an include file. Not a huge deal since the code length is so small, but something to consider.
  • I'm confused by the skipto_form_submit handler. It seems to be used to add "[role=", etc. because the JS config needs that to run Landmark related code properly. Seems to me it would be easier to have that as actual stored config or to add some processing code to your hook_init() implemenation
  • The custom submit handler also seems to be used to return confirmation of settings. I would think that automatic confirmation provided by a system_settings_form would be sufficient. If you need specific data storage or messaging that system_settings_form doesn't provide you might be better off rolling your own.
  • Consider throwing a warning on the settings screen when the required JS plugin is missing.
ronfeathers’s picture

ronfeathers commented
Status: Needs work » Needs review

THANK YOU for the suggestions, @chrisfree
I've addressed most of your thoughts - except skipto_form_submit change. I wanted to not have to add the role=' stuff in the variable, as I'm hoping to get that removed in the SkipTo JS repo soon. Then, the formatter will go away but the var will remain intact.
Again, Thank you for your review!
~R~

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Assigned: Unassigned » sreynen
Status: Needs review » Reviewed & tested by the community
Issue tags: -PAreview: review bonus

manual review:

  1. skipto_form(): in that case it does not make sense to run the #options values through t(), right? This is a known false positive from Drupal practice sniffer.
  2. note the code lines do not have to be under 80 characters in length (only comments). Line breaking strings in t() is not a good idea.
  3. skipto_form_submit(): "if (isset($_POST)) {": why is that needed? Please add a comment.
  4. "if (module_exists('libraries') && function_exists('libraries_get_path')) {": that does not make sense, since you depend on libraries module you can assume it is always there.

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

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

ronfeathers’s picture

ronfeathers commented

1. removed t() from #options values
2. line breaks removed from t() strings
3. yeah - wasn't needed I guess in hook_form_submit() :) removed.
4. removed also, unnecessary.

THANK YOU for reviewing, @klausi

~R~

ronfeathers’s picture

ronfeathers commented
Issue summary: View changes

adds more reviews for bounty

ronfeathers’s picture

ronfeathers commented
Issue tags: +PAreview: review bonus

adds "PAReview: review bonus" tag

ronfeathers’s picture

ronfeathers commented
Issue summary: View changes

adds more review comments

ronfeathers’s picture

ronfeathers commented

Changed config link from /admin/content to /admin/user-interface where it seems to make more sense.
Thanks,
~R~

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Reviewed & tested by the community » Fixed

no objections for more than a week, so ...

Thanks for your contribution, ronfeathers!

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:

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.

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Moved link from /content to /user-interface

mpnkhan’s picture

mpnkhan commented
Issue summary: View changes