[D7] Vertical Tabs Responsive

Hey, @rcodina. You have given the command to your personal repo. The command for everyone else should be something like:
git clone http://git.drupal.org/sandbox/rcodina/2301115.git

Also, this module doesn't contain much code - 16 lines of php that are used to unset two files, and then one very small js function, so I don't think this qualifies for evaluation.

Yes, @rcodina, but I still don't think this is eligible for review, or at least not to allow you to receive the "promote to full project" permission. The rule is that a module should contain at least 5 functions, and 80 + lines of code, minimum, iirc.

Have a look here, @rcodina: https://groups.drupal.org/node/195848
I misspoke when I said 80 lines - the rule of thumb is 120. I'm pretty sure that css doesn't count towards this tally, as you aren't building functions and making use of drupal api's when styling. What is needed is a certain amount of php, first and foremost, and then if you want to use some js towards this requirement, that *may* work (not sure). Either way, there really isn't anywhere near enough here to evaluate your level of ability and adherence to drupal logic and best coding practices.
"IIRC" = "If I remember correctly."

All of this said, this module may be promoted to a full project if there is a community need, but this module does not meet the minimum requirements for you to be given the permission to promote your projects from sandbox to full projects.

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

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.

Hey @rcodina - you are still kind of code light, imho, but I like the idea of building out your admin form a little bit more to overcome this. You should add more form options, as opposed to a simple on off kind of toggle. Adding some logic to said form will quickly get you over the lack of code hump.

1) PAReview is clean
2) Functionally works as expected, although you might consider some css cleanup (widths, etc.)
3) Please use t() for text in .module lines 15, 16, 26, 27
4) Not a show stopper, but you could make your .js file more concise. For example:
Instead of:

jQuery('body[class~="page-node"] #content').css('width', 'initial');
jQuery('body[class~="page-node"] #content').css('margin-left', 'initial');

Do:

jQuery('body[class~="page-node"] #content').css({
         'width': 'initial',
         'margin-left': 'initial'
});

Yep, my bad. Looks good to me.

Yes, those css changes are fine. Now you have to wait for others to review your module, and a git administrator to promote this to a full project. Looking through the issues queue, you'll see that this process can take a considerable amount of time.

@rcodina, thankyou for your contribution!

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. None.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes. If "no", list security issues identified.

Coding style & Drupal API usage

1. (+) vertical_tabs_responsive_admin_settings: This setting form contains separate submit function, even you are also using system_settings_form(). Also in your submit function you are using variable_get() that system_settings_form does itself. Your code should be something like this.
   

   function vertical_tabs_responsive_admin_settings() {
   
     $form['vertical_tabs_responsive_left'] = array(
       '#type' => 'checkbox',
       '#title' => t('Show vertical tabs on left side.'),
       '#default_value' => variable_get('vertical_tabs_responsive_left', 0),
     );
   
     $form['vertical_tabs_responsive_content_width'] = array(
       '#type' => 'textfield',
       '#title' => t('Content width.'),
       '#description' => t('Enter percentage.'),
       '#default_value' => variable_get('vertical_tabs_responsive_content_width', '70'),
       '#size' => 2,
       '#maxlength' => 2,
       '#required' => TRUE,
       '#field_suffix' => t('%'),
     );
   
     $form['vertical_tabs_responsive_vt_width'] = array(
       '#type' => 'textfield',
       '#title' => t('Vertical tabs width.'),
       '#description' => t('Enter percentage.'),
       '#default_value' => variable_get('vertical_tabs_responsive_vt_width', '30'),
       '#size' => 2,
       '#maxlength' => 2,
       '#required' => TRUE,
       '#field_suffix' => t('%'),
     );
   
     $form['#validate'][] = 'vertical_tabs_responsive_admin_settings_validate';
     return system_settings_form($form, TRUE);
   }
   

   Just removed the extra submit function, it would work in similar as you required.

2. hook_help() is missing in your module.
3. (+) vertical_tabs_responsive_page_build: You are adding js/css files using drupal_add_js/drupal_add_css rather use #attached to add the JS/CSS file with page rendered array where these actually required.
4. vertical-tabs-custom.js:Use Drupal.behaviors, not jQuery(document).ready() in your js file.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

As I am not a git administrator, so I would recommend you, please help to review other project applications to get a review bonus. This will put you on the high priority list, then git administrators will take a look at your project right away :-)

Thanks again for your hard work.

One more point in vertical_tabs_responsive_page_build: you are passing php values in your js file using following code.

// Passing variables to javascript.
  drupal_add_js("var vertical_tabs_responsive_left = " . drupal_json_encode(variable_get('vertical_tabs_responsive_left', 0)) . ";
    var vertical_tabs_responsive_content_width = " . drupal_json_encode(variable_get('vertical_tabs_responsive_content_width', 70)) . ";
    var vertical_tabs_responsive_vt_width = " . drupal_json_encode(variable_get('vertical_tabs_responsive_vt_width', 30)) . ";", 'inline');

This is not a recommended approach in drupal, better to pass these variables in following way:

You can add variables in the Drupal.settings object like the following syntax :
drupal_add_js(array('module_name' => array('base_path' => base_path())), 'setting');

You can have the base_path variable in your js file as mentioned below :
alert(Drupal.settings.module_name.base_path);

For more information you can visit to following URLs:

1. https://www.drupal.org/node/756722
2. http://drupal.stackexchange.com/questions/23132/sending-a-variable-to-ja...
3. http://drupal.stackexchange.com/questions/4482/how-to-send-variable-from...

Hi,

Thanks for the contribution, mean while some small suggestions if you like to update.

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.

No duplication
Yes
Master Branch
Yes: Follows the guidelines for master branch.
Licensing
Yes: Follows the licensing requirements
3rd party code
Yes: Follows the guidelines for 3rd party code.
README.txt/README.md
Yes
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code
YES
Coding style & Drupal API usage

1) Not sure what the second paramter does
system_settings_form($form, TRUE), becuase in drupal 7 it excepts only one paramter

2) If you are creating a local scope $ variable for Jquery then i think it is better to use $ than jquery
(function ($) {

})(jQuery);

3) General practice is that
if (left != undefined) we use

if (tyepeof left != 'undefined')

4) Is there any reason why you are using json encode on a string?
drupal_json_encode(variable_get('vertical_tabs_responsive_content_width', 70))

5) is it really necessary to create invisible fieldset as below
$form['vertical_tabs_responsive'] = array(
'#type' => 'fieldset',
'#title' => t('Vertical tabs responsive'),
'#collapsible' => TRUE,
'#collapsed' => FALSE,
'#tree' => TRUE,
'#weight' => 99,
'#group' => 'additional_settings',
'#attached' => vertical_tabs_responsive_get_attached(),
'#attributes' => array('class' => array('element-invisible')),
);

Instead cant we use as mentioned in the documentation.

$form['#attached']['css'] = array(
drupal_get_path('module', 'ajax_example') . '/ajax_example.css',
);

$form['#attached']['js'] = array(
drupal_get_path('module', 'ajax_example') . '/ajax_example.js',
);
$form['#attached']['css'][] = array(
'data' => 'YOUR_INLINE_CODES',
'type' => 'inline',
)

$form['#attached']['js'][] = array(
'data' => 'YOUR_INLINE_CODES',
'type' => 'inline',
)

6) is it necessary to check if (path_is_admin(current_path()) && vertical_tabs_responsive_should_responsify_vt()) { inside vertical_tabs_responsive_get_attached becuase if you are calling the function vertical_tabs_responsive_get_attached only after that check in the parent function.
7) Not sure if the below code is really necessary in hook_help
default:
$out = '';
break;

8) I think it would be better if you could combine these 3 array elements into 1 array element.
$attached['js'][] = array(
'data' => array('vertical_tabs_responsive' => $left),
'type' => 'setting',
);
$attached['js'][] = array(
'data' => array('vertical_tabs_responsive' => $content_width),
'type' => 'setting',
);
$attached['js'][] = array(
'data' => array('vertical_tabs_responsive' => $vt_width),
'type' => 'setting',
);

Prajwal

Thanks looks to good to me. We all are learning. Keep up the good work

  $items['admin/config/vertical_tabs_responsive'] = array(

All admin items MUST be below one of the config sub-sections.

  if ($form_state['values']['vertical_tabs_responsive_content_width'] + $form_state['values']['vertical_tabs_responsive_vt_width'] != 100) {
    form_set_error('vertical_tabs_responsive_content_width', t('Content width plus vertical tabs width must equal 100%'));
  }

If these two settings MUST add up to 100%, then why ask the user for both? Surely you can ask me for just one and calculate the other!

  $form['vertical_tabs_responsive_left'] = array(
    '#type' => 'checkbox',
    '#title' => t('Show vertical tabs on left side.'),
    '#default_value' => variable_get('vertical_tabs_responsive_left', 0),
  );

This is not a good UI pattern. Checkboxes should be used for 'on/off' states, but this doesn't fit that case. Here you force the user to think about what 'off' means. It would be better to have a dropdown with two options: left and right.

function vertical_tabs_responsive_admin_settings() {

Missing form builder params.

 * Configuration form render.

The word 'render' is redundant.

/**
 * Returns true if CSS and JS of this module should be added on currents page.
 */
function vertical_tabs_responsive_should_responsify_vt() {
  $is_node_add = ((arg(0) == 'node') && (arg(1) == 'add'));
  $is_node_edit = ((arg(0) == 'node') && (arg(2) == 'edit'));

Needs a @return.

Also, if you're calling arg() lots, just call arg() without the parameter and look in the returned array yourself.

Lastly, you're calling this function at least 3 times in a single request. Statically cache the return result so you don't have to calculate it each time.

  if (vertical_tabs_responsive_should_responsify_vt()) {
    unset($javascript['misc/vertical-tabs.js']);
  }

Though if your aim is to always change vertical tabs, can't you just assume that if $javascript['misc/vertical-tabs.js'] is set, then you should unset it?

/**
 * Some help for users.
 *
 * Implements hook_help().
 */

The 'implements' line should always be first.

Finally, I tried enabling this module with my own custom theme, and it doesn't work! Vertical tabs get turned off and I just see fieldgroups.

> Just now my module is only compatible with following administration themes: Seven, Adminimal and Commerce Kickstart Admin Theme

I'm not a themer, so this might be a stupid question, but why does it only work with specific themes?

> No, I can't because I only responsify them in node add/edit pages and pages under admin/structure.

Also maybe a stupid question, but can't it change all vertical tabs?

> No because I discovered that some modules/themes use "full width" vertical tabs as form. In theses cases, my module would mess the form and turn into an unusable thing. Just install bootstrap theme and go to theme preferences. You will see an example of this kind of forms.

I've had a look at this, and I'm not sure what you mean by 'full width'. How is the Bootstrap theme settings use of vertical tabs different from node edit?

> whole form are vertical tabs

Is it possible to detect this form structure from the form array? That would mean you can drop the specific form ID detection.

It works for me. I think he can do a beta version because then a lot of more users will give him feedback.

Since we are quite busy handling all the project applications you should take part in the Review bonus system. This system rewards people who are willing to help out with a bonus in the form of a prioritized application queue.

Thanks.

fixing tag.

manual review:

• vertical_tabs_responsive_help(): all user facing text such as the help text must run through t() for translation.

But otherwise looks good to me, so ...

Thanks for your contribution, rcodina!

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.

You have the git vetted user role, so that seems to be in order. Please open a webmaster issue at https://www.drupal.org/node/add/project-issue/webmasters