This module replaces AJAX loading spinners (aka throbbers) with an extendable and CSS-styleable subsystem.

Currently it integrates with the external libraries Raphael or Spin.js to draw throbbers.
There is a "raphael" module, but the library included is outdated. I created a patch, but until "raphael" has been updated, these two modules will conflict.

Hurricane uses the Libraries API to load external libraries.

Project page: http://drupal.org/sandbox/pmelab/1806234

Git clone:

git clone http://git.drupal.org/sandbox/pmelab/1806234.git sites/all/modules/hurricane
git clone https://github.com/DmitryBaranovskiy/raphael.git sites/all/libraries/raphael
git clone https://github.com/fgnass/spin.js.git sites/all/libraries/spinjs

Neither Spin.js nor Raphaël are required for Hurricane to work. They are used in optional, prepackaged rendering plugins. But for testing the module, you will want to download them.
Hurricane is built to integrate any other library able to draw throbbers. Or no external library at all, if you want to build the javascript animation from scratch (as shown in drop.js).

Comments

r2integrated’s picture

r2integrated commented
Assigned: Unassigned » r2integrated
Status: Needs review » Needs work

Hey there, thanks for the application!

Unfortunately I am unable to proceed with reviewing your application due to some bugs. I installed the module as per your instructions however whenever I attempt to perform any AJAX operations, such as adding a field to a view, however I am receiving an error popup. I've traced the point where the error is caught to misc/ajax.js line 258. The error is a TypeError, thrown because some Javascript code, presumably in your module, is attempting to call a function which is undefined. Please investigate and fix so we can evaluate the module.

PAreview came back almost clean:

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. Get a review bonus and we will come back to your application sooner.


FILE: /www/vhosts/sandbox.localhost/sites/all/modules/hurricane/README.txt
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 20 | WARNING | Line exceeds 80 characters; contains 81 characters
--------------------------------------------------------------------------------
pmelab’s picture

pmelab commented

Thanks for (trying) to review!

I've found a bug that produced the error you described. If hurricane_integration is enabled but no throbbers where activated in the themes settings, it tried to call undefined. Has this been the case? Should be fixed now.

http://ventral.org/pareview thinks line 20 of the README.txt is 80 characters long, not 81.
Odd

pmelab’s picture

pmelab commented
Assigned: r2integrated » Unassigned
Status: Needs work » Needs review

Forgot to change status to "needs review" ...

r2integrated’s picture

r2integrated commented
Status: Needs review » Needs work

Overall:
Your fix worked, I can review it now! There are some relatively minor issues documented below, but overall this is really well done. One question I have though - is it stated clearly anywhere which to use - Raphael or SpinJS? I'm able to install both modules but I am unsure of which is being utilized.
Automated Review:
Clean.

Manual Review:

  1. No .install provided, settings are not removed during uninstall.
  2. hurricane.info, hurricane_integration.info, hurricane_raphael.info, hurricane_spinjs.info, hurricane_tests.info - package should be "User interface" not "user interface", or another package of your choosing just so long as you capitalize the first word.
  3. hurricane_tests.module - If this module is enabled, the hurricane/test callback is available to anonymous users. The ajax callbacks include calls to sleep(). Sufficiently knowledgeable users could determine the URL for these callbacks and access them in rapid succession, resulting in a sort of DOS attack. The simplest way to mitigate this potential attack vector would be to limit access to these callbacks.
pmelab’s picture

pmelab commented
Status: Needs work » Needs review

Thank you!

@ Overall:
It depends on the developers choice between "beauty" and filesize. And ultimately they are proof of concept that completely different technologies are possible. I added additional information in the Usage section to the README.txt and project page.

@ Manual Review:

  1. Hurricane uses only theme settings. How am i able to remove them? Should I remove them? Don't they belong to the theme? They could be stored in the themes info file too. I already thought about this, any advice would be welcome.
  2. Package names are fixed.
  3. Test page and ajax callback are now restricted to users with administer themes permission, since they are most likely to use it.
dasjo’s picture

dasjo
he/him
Primary language German
Location Zurich
commented

hi,

congratulations - this feel like a very solid project application.

i have tested all the sub-modules provided and found them to be working just fine both on chrome/mac + chrome/android.

here are some remarks:
- hurricane.info states "Raphael based loading indicators" as description. As raphael is one possible integration, you might rather name it something like "Javascript based and CSS-styleable loading spinners."
- would be great to have a hurricane.api.php that documents hook_hurricane_renderers()
- a little cleanup patch goes here: #1865162: Minor code cleanup

i'm leaving this on needs review as i'm not very familiar with how strict the application process is. from my point of view the above remarks shouldn't prevent this already very ready module to be released.

jphelan’s picture

jphelan commented
Status: Needs review » Needs work

This is a great module. Well done.

Manual Review:

  1. I agree with dasjo - hurricane.info states "Raphael based loading indicators" as description. As raphael is one possible integration, you might rather name it something like "Javascript based and CSS-styleable loading spinners."
  2. The enabling process fells a little odd to me. What are the check boxes for? If I check spin.js and hit save it gives me the select with both drop and spin.js as options but drop was selected. Why does drop show up in the select if it's not enabled?
  3. I could not get Hurricane Spin.js to work without enabling Hurricane Integration. Should it be a dependency?
pmelab’s picture

pmelab commented
Status: Needs work » Needs review

Thanks for the review!

@ 1.
Finally fixed that. Somehow I managed to forget about it.

@ 2.
There's been a bug that loaded all available renderers if you check only one checkbox. Fixed.

The checkboxes are responsible for loading specific renderers for a theme, which then can be chosen by setting the font-family CSS property on the throbbers parent element. This section appears even without hurricane_integration enabled, if you choose to attach the throbbers manually by loading the library (drupal_add_library('hurricane', 'hurricane');) and and using the jQuery plugin ($.hurricane()).

@ 3.
All renderers work without hurricane_integration, but they won't appear anywhere. Thats what the integration module does. I added a note to the README.txt and project description.

jphelan’s picture

jphelan commented
Status: Needs review » Reviewed & tested by the community

Great with @2 fixed, it works as expected.

The only other thing I have is just a suggestion. The reset button did not do what I excepted it to do, I thought it would reset the the selected renderer options back to their defaults not disable everything. I would suggest either adding a note to what reset does or change it to the behavior I mention. Just a suggestion though.

Great module, I will probably be using it on an upcoming project.

pmelab’s picture

pmelab commented

Good point. Doesn't make sense to disable all renderers, since there are no real "default" values. Fixed in the latest commit.

jthorson’s picture

jthorson commented
Status: Reviewed & tested by the community » Fixed

Just one additional comment:

hurricane_spinjs.module: Lines 14-17
hurricane_raphael.module: Lines 61-64
See http://drupal.org/node/322774 for the preferred way to include links in your strings within t(), which ensures that a translator has access to the full context of the sentence while translating.

Thanks for your contribution, pmelab!

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.

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Added a note about library requirements, or the lack of them.