[D7] Ninesixty block helper

Hi Ujin,

- The module contains some syntax issues:
http://ventral.org/pareview/httpgitdrupalorgsandboxujin1953154git
- I don't if know the module is substantial enough. It looks to me it's just the definition of some permissions and altering an existing form.
- I did not use the ninesixty theme, but didn't receive any warning.

Hi Ujin,

As mentioned by beljaako ventral shows several issues that should be attented. The most important ones are that you are still working on the master branch (how you should switch) and that you don't have a readme file. (some tips for your readme file)

I took a look at your code and noticed the following issues around the variable ninesixty_settings:

• I checked the Ninesixty theme and they don't define the variable, so if you are the one defining it should start with the name of your module. (ninesixty_block_helper_settings)
• You don't have a .install file which should contain the hook_uninstall() function and preferably hook_install() as well. In hook_uninstall() you should remove your defined variables with variable_del();

And line 94 and 95 are identical:

  $block_settings = variable_get('ninesixty_settings', array());
  $block_settings = variable_get('ninesixty_settings', array());

Also your project page needs a little more work, you can find some tips here. ;)

Greetings!

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and 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.

Ujin,

Thanks for taking the time to update your module.

You have a nice use of hooks in this module and the documentation around the functions appear to be inline with the coding standards.

I'm not a themer and am not sure that I would ever use this module simply because I'm not sure what it buys me. I'm sure it is useful and some more text on the project page (and possibly a screenshot) would help others understand why they would want to install this module.

Great! Thanks for switching and using the 7.x-1.x git branch. It is so helpful when trying to determine what version of the module users are trying to install. As a follow up to that, please remove the master branch as to not create any confusion about where the most up-to-date code lives.

• There is a misspelling in the comment on line 96 and the text starting with "Add ninesixty..." should be on a separate line.
• Please wrap your title text (lines 45 & 51) in the t() function so they may be translated as per this doc.

The only thing that might be a blocker is this module does contain only 4 functions with <120 lines of code. I'm not sure how particular they will be about it.

Hi, my manual review:

• Use empty line between functions
• remove line 27 $pad_options = array(); - it is unnecessary
• Please use formatted code here:
  

      if (isset($form_state['values']['grid_class']) &&
          ($form['settings']['grid_class']['#default_value'] != $form_state['values']['grid_class'] ||
              $form['settings']['grid_pad']['#default_value'] != $form_state['values']['grid_pad']) &&
          user_access('administer blocks')) {
  

• You have use too long code strings http://ventral.org/pareview/httpgitdrupalorgsandboxujin1953154git. Please split it.
• Wrong comment format:
  

  /**
    * Implements theme_preprocess_block(). Add ninesixty giid and padding classes.
    */
  

• Remove "block" dependencies dependencies[] = block This module exist by default.

Have a nice day!

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

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