[D7] Collapsible list module

Fixed the git clone URL in the issue summary for non-maintainer users.

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.

This module is written by coding standards but it is not user friendly.
I suggest you to add some administration pages so it would be more easy for use.

Hi Goska

Thanks for you contribution, i have a few observations...

• .module:56 use drupal_attributes rather than $output = '<div class="item-list ' . $wrapper_class .'" data-js="collapsible-list">';
• Not a problems per se, but in conditionals, use string == variable rather than variable == string (replace $key == ‘data’ with ‘data’ == $key), this demonstrates defensive coding (should you mistakenly miss off a = then $key = ‘data’ is still valid syntax whereas with ‘data’ = $key is a syntax error).
• Maybe theme_collapsible_list should be building $output as a render array rather than string concatenation.
• $type, should only be passed to theme_item_list, never added in this module.
• Use theme_item_list in all cases, just alter $attributes for first and last li rather than manually adding li. It appears in its current state that you will end up with nested ul/ol tags (one from theme_item_list, one from lines 70/120.
• Remove drupal_add_js(drupal_get_path('module', 'collapsible_list') . 'js/collapsible_list.js');, this file should already be present from your entry in the .info file.
• A nice to have would be to validate $type (only allow ul, ol, etc) if invalid use default and add watchdog warning.
• $show_text and $hide_text should be wrapped in t() to allow default (and possibly supplied) values to be translated.

Kind regards

Andy

Hi,
Below are my review

@rajveergang: Removing review bonus tag, this should only be added to your own application. Can you check that you only added it there?

@klausi,

My mistake.

Hi Gosia,

This looks like a really useful module, just a couple of things to update:

In collapsible_list.module:

26: Title should be translatable e.g. wrapped in the t() function.
30 & 31: 'View all' and 'View less' need to be translatable e.g. wrapped in the t() function.

In collapsible_list.js:

There are quite a few references that suggest the JavaScript is being used for a particular type of list e.g. a documents list. I would suggest changing things like 'totalDocsToShow', 'document-list' and 'totalDocuments' to generic names e.g. itemList.

Ah I see you've translated the 'view all' in the JavaScript but not the 'view less'. Might be an idea to do the translation in the .module file rather than the JavaScript.

There are quite a few hard coded classes in the JavaScript which use BEM naming convention. Suggest providing options to allow developers to override these classes if they need to rather than assuming people are using BEM.

The destroy method looks like it needs to remove the the aria, tabindex and id values you are adding on the list and list items.

In the readme:

You could add which version/s of jQuery this is compatible with.

Suggestions for future improvement:

- It might be nice to make the title of the list into a link, or at least provide this as an option.
- Have you thought about including definition lists?

Hi @goska_m

This looks really good, cant see any further issues.

Hi @goska_m

I think you need to account for the possibility than one collapsible list may occur on a single page, so the settings need to be applied per list.

Thanks
Andy

Hi goska_m,

I have review on your module and found:

1. Firstly you have to fix your https://pareview.sh/node/661 that is currently have error like https://www.screencast.com/t/O7RXpeIp8
2. If your module is having dependency with jQuery update add this to info file like you have mention this on help https://www.screencast.com/t/Bf0jE1vgJTF
3. There are no configuration setting for your module so it's difficult to understand how it works
4. in the collapsible_list.info: why you are adding js file to all the pages, add this where it's need

Regards
Satyam

Manual Review

Hi @goska_m

I have review on your module and I suggest the following changes:

The examples in project page is wrong, in the page documentation is the following code;

$variables = array(
  '#items' => array('item1', 'item2', 'item3', 'item4', 'item5'),
);
theme('collapsible_list', $variables);

But in your module, you are manipulating the variables without #;

function theme_collapsible_list($variables) {
  $output = array();
  $items = $variables['items'];
  $title_text = $variables['title_text'];
  $title_wrapper = $variables['title_wrapper'];
  $title_path = $variables['title_path'];
  $title_options = $variables['title_options'];
  $type = $variables['type'];
  $cutoff = $variables['cutoff'];
  $wrapper_class = $variables['wrapper_class'];
  $control_class = $variables['control_class'];
  $list_class = $variables['list_class'];
  $list_item_class = $variables['list_item_class'];
  $attributes = $variables['attributes'];
  $show_text = $variables['show_text'];
  $hide_text = $variables['hide_text'];

You should correct the documentation or the code.

[JS]
You should call the plugin inside Drupal behaviors, it's possible you add the context, just a good recommendation, because after Drupal ajax is called all behavior to bind the new elements.

(function ($) {
  Drupal.behaviors.pluginCollapsibleList = {
      attach: function (context, settings) {
        $('[data-js="collapsible-list"]', context).collapsibleList(settings);
      }
  }
})(jQuery);

This a good article https://www.lullabot.com/articles/understanding-javascript-behaviors-in-... about it.

Hi @goska_m

I send a patch with some modifications in your JS file(collapsible_list.js).
I organized the functions. Now it actually works as a plugin without having dependency on template settings.

You can try to destroy the plugin so easily, just call the below code:
jQuery('[data-js="collapsible-list"]').collapsibleList('destroy');

And call again to bind the plugin:
jQuery('[data-js="collapsible-list"]').collapsibleList();
And if call again the plugin, it not create a other button, the plugin will be use the same instance, this is great for performance.

I think this is a great contribution to your module, but it needs to be revised, so I'm going back to Needs Work.

Please consider this patch, thank you.

Both local and pareview.sh pass without an issue.
Manual review
I didn't find something but in collapsible_list.js i would advise that you add some comments to help work with it in the future.

@Drupal8: looks like you forgot to change the status. Anything else that you found or should this be set to RTBC?

Code reviewed return no issue.

Thank you for your contribution!

I updated your account so you can opt into security advisory coverage now.

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!

Thank you, 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 go the dedicated reviewer(s) as well.