[D7] Smallipop

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.

Added Drupal version.

Hi, hapax legomenon

Whitespace found at end of line 67:

FILE: /var/www/drupal-7-pareview/pareview_temp/smallipop.module
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 67 | ERROR | Whitespace found at end of line
--------------------------------------------------------------------------------

Nice little jquery lib.
Here's a small issue I ran into while installing...
The README explains that the path for the library should be at "sites\all\libraries\jquery.smallipop\lib\jquery.smallipop.min.js" but putting it in that location I got a File Not Found. Looking at the page source I saw that it was looking for the file here "sites\all\libraries\smallipop\lib\jquery.smallipop.min.js"

Thanks for your contribution!

Hi,

Automated review:
PAreview.sh on ventral.org: http://ventral.org/pareview/httpgitdrupalorgsandboxmrasia1995350git... 0 problems found.
Coder Drupal module: OK, nothing found.

Manual review: (In the manual review I have corrected the error of No #4 manually)
- Install a fresh drupal 7.22
- Move the module package "smallipop" to folder to sites/all/modules (no problems found)
- Copy the smallipop libs in the corrected path (see #4)
- Move the module package "jquery_update" to folder to sites/all/modules (no problems found)
- Move the module package "webform" to folder to sites/all/modules (no problems found)
- Enable jquery_update module and configure jQuery version 1.8 (no problems found)
- Enable webform and smallipop modules (no problems found)
- Check with Firebug / Firefox v. 21.0 if the js files are included in drupal (no problems found)
- Create a test webform with two input fields and the corresponding input field descriptions (no problems found)
- Save test webform (no problems found)
- Check with Firebug / Firefox v. 21.0 whether the css class of the field has been added (=> class="smallipop-input" / no problems found)
- Tested and played with the settings, but unfortunately I could not find the desired function. There are no tool tips displayed.
- Tested browsers => Chrome v. 27.0.1453.93 / Firefox v. 21.0 / Safari v. 6.0.3 (8536.28.10) (there are no tool tips displayed)

Summary: All the CSS definitions and js scripts have been loaded and the CSS class is added to the input field, but I could find no functionality. The console also provides no js error, so I can not say anything else more accurate. Did I forget something in the installtion? Could you please complete the README with installation instructions and application description.

Best regards.

status => needs work

Big issues:

• In hook_library(), you should include settings within the smallipop library definition: see below. As it stands, you're adding javascript when the library hook is invoked, NOT when the library is included.
• Your if statements on lines 64 and 69 can be combined into a single statement, as they run the same code.
• You are passing all of your variables as strings to the javascript (they're also stored as strings, but storage alone doesn't cause an issue), which is causing problems with the popup position. You need to make sure you're passing the right types or the module doesn't work as intended.

Defining settings in a library:

$libraries['smallipop'] = array(
    'title' => 'Smallipop',
    'website' => 'https://github.com/Sebobo/jquery.smallipop',
    'version' => '0.5.2',
    'js' => array(
      SMALLIPOP_DEFAULT_PATH . '/lib/jquery.smallipop.min.js' => array(),
      array(
        'type' => 'setting',
        'data' => array('smallipop' => $settings),
      ),
    ),
    'css' => array(
      drupal_get_path('module', 'smallipop') . '/css/jquery.smallipop.css' => array(
        'type' => 'file',
        'media' => 'screen',
      ),
    ),
  );

Minor issues:

• In smallipop.js, you should remove the comma after the last line in your options object. Trailing commas are supported in php, but not javascript.
• Since this module is meant to provide integration with an outside library, you shouldn't include a version of a file from that library within the module (as it may be updated later). As such, jquery.smallipop.css should be removed from the module and instead loaded from the libraries folder.
• You should mention webform integration within your README.txt file, and in the project description.

Added whitespace.

Please add all your review comments to the issue summary so that we can track them.

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/smallipop.module
  --------------------------------------------------------------------------------
  FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   89 | ERROR | No key specified for array entry; first entry specifies key
  --------------------------------------------------------------------------------
  

• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: /home/klausi/pareview_temp/smallipop.admin.inc
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 9 WARNING(S) AFFECTING 9 LINE(S)
  --------------------------------------------------------------------------------
    64 | WARNING | #options values usually have to run through t() for
       |         | translation
    65 | WARNING | #options values usually have to run through t() for
       |         | translation
    66 | WARNING | #options values usually have to run through t() for
       |         | translation
    80 | WARNING | #options values usually have to run through t() for
       |         | translation
    81 | WARNING | #options values usually have to run through t() for
       |         | translation
    82 | WARNING | #options values usually have to run through t() for
       |         | translation
    83 | WARNING | #options values usually have to run through t() for
       |         | translation
   133 | WARNING | #options values usually have to run through t() for
       |         | translation
   134 | WARNING | #options values usually have to run through t() for
       |         | translation
  --------------------------------------------------------------------------------
  

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. The Git commits are not connected to your user account. You need to specify an email address. See http://drupal.org/node/1022156 and http://drupal.org/node/1051722
2. smallipop_init(): hook_init() is executed on every single page request, even if it is just a redirect, a web service call or whatever. I think you may want to use hook_page_alter() or hook_page_build() instead?
3. "'access arguments' => array('administer configuration'),": that permission does not exist. Did you mean "administer site configuration"?
4. _smallipop_library_settings(): why do you need so many variables, you store all the tiny settings in one settings array in one variable?
5. "$form_names['all'] = 'All';": all user acing text must run through t() for translation. Same for "'black' => 'Black'," in #options, please check all your strings.

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

Assigning to dman, as he might has time to take a final look at this.

Looks like a good project.

Approaching it with a clean mind to give you my first-time-user impressions..

• Built a brand-new clean site with 'minimal' profile.
• Enabled smallipop via the UI. This auto-required "libraries" and "jquery_update". I see this is noted on the project page already, so I'm happy about that. Those are easy to enable.
• Was immediately able to see the Smallipop config page showing up under 'Configuration' , so that's as good as can be expected.
• It complained that the library was not found, but didn't tell me what to do about that. So I look for the README, and yes, it's nice and clear there. Lovely.

cd sites/all/libraries
wget https://github.com/Sebobo/jquery.smallipop/archive/master.zip -O smallipop.zip
unzip smallipop.zip
mv jquery.smallipop-master/ smallipop
ls smallipop/lib/

Should do it..
Complaint seems to have gone away now.

So far you've been referring to 'smallipop' as if everyone knows all about exactly what it is. The project page makes no effort to explain what it does or why you'd want it. I'm now looking at all the settings on the config page and I'm still no wiser - except it does things to form titles.
I support and train clients and developers and content managers. I troubleshoot for folk that pick up an existing site that may have been built a year or three ago and they have to understand or audit what is on it and why. Looking at this, I recognize things that would confuse them. Right now this is just jargon.
Just a one-liner of help on the top of the config page that describes what this module does in human terms would be very helpful here.

* so now, I save the configuration. Apparently it's now attaching itself to ALL forms. Yet I see no change anywhere. Where should I be looking
?
Lets check that README again.
Sooo - it's specific to "webforms" only? Like ... just webform.module? It doesn't affect other Drupal FAPI web forms?
Hm. It said nothing about that on the project page - and other tooltip libraries (beautytips, field_tooltip, formtips ) already doing this job do actually "integrate with Drupal" - not just webform.
I'd have thought that if you got FAPI integration working first, then webform support would come free from that. Hm.

OK. So this module does nothing without webform.module - yet it's not listed as a dependency.

• Enable webform.module and see where that gets us... I have webform 7.x-3.18 handy, so lets try it.
  • Made a webform, added three fields. text, date, text.
  • Put descriptive help in each of the filed settings.
  • The hover tip showed up!
  • ... but would not go away. There seems to be no way to dismiss it with the default settings, and it made the form unusable immediately. Cannot "esc" or tab it away, cannot make it go away by clicking. Cannot see the next form field. :-(

** well, that sort of mess-up can probably be fixed by using the settings correctly - I'll see there...
Hm, if showing up underneath was position:top, I wonder what position:bottom is? Nope. doesn't make a difference..
Let's try "right" .. :-(

Oh dear.

Well, as a utility, it certainly looks like it needs a bunch more testing to actually work right.
I'll stop user-testing now, because it's a work-in-progress. Lets look at the code.

The automated reviews above have probably caught everything technical.

• correct use of Drupal.behaviors and Drupal.settings
• correct use of hook_ininstall to clean up the variables
• correct use of hook_requirements
• nice use of form states in the config form.

I'm not happy with smallipop_form_alter(). I've read it through 4 times now and I can't see why it's using a variable called smallipop_attach_webform_class_toggle_form but running on ALL system forms ever, why it's only modifying the form when the form is submitted... or what it's doing at all. It looks very confused. Shouldn't you be checking if it's of type 'webform' somewhere there?

You are adding the javascript library to the page via hook_page_build() and drupal_add_js() when you detect the current_path.
Though this may be working in simple cases, there are parts of Drupal (caching, overlay, AJAX loads) where this will start to fall apart - in tricky places.
What you should be doing instead is using FAPI #attached and attaching your library directly to the render array of the form.
You are already capturing hook_form_alter and doing ... something ... to it. Add your library then.

This would mean that other webform-modifiers (and there are a lot of them) will not break what you've done (so much). EG, there is webform-in-a-block, or webform-in-a-panel uses that your logic will currently fail on, but can be made to work : if you attach your form manipulation to the form, not the current_path.

So this gets a B-minus from me.
* Doesn't actually perform well right now
* Is opaque about its form_alter
* should use the Form API better in order to be compatible with other modules (but you can't be expected to know all that yet)
But it's still a pass. It's not awful in any way.

I won't let this review hold up the process, I think we can promote this account ..

(There seems to be a small snafu with our ability to promote accounts - related to the security lockdown earlier this month. I can't promote immediately sorry.)

Thanks for your contribution!

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:

• 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 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.

What happened to this module?? https://www.drupal.org/project/smallipop returns 404.

Apparently Smallipop now supported by Tips: https://www.drupal.org/project/tips