QueryLoader2 integration [#1360332]

Description
Enables a page preloader (QueryLoader2) on a site for given paths using settings provided in the administration area. Uses Drupal 7 core libraries hook as well as Drupal behaviors to embed the plugin. All code was checked on Coder and is Drupal coding standards compliant. I've also been in contact with the author of the plugin who has given me his blessing to make the module.

The module isn't rocket science but nevertheless I think it'll be useful for sites with pages heavy on UI.

Project page
http://drupal.org/sandbox/interactivejunky/1360320

Drupal version
Drupal 7

GIT
git.drupal.org:sandbox/interactivejunky/1360320.git

Comments

Comment #1

patrickd commented 3 December 2011 at 21:54

Status:

Needs review

» Needs work

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the master branch:

Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards):


FILE: ...ew/sites/all/modules/pareview_temp/test_candidate/queryloader.admin.inc
--------------------------------------------------------------------------------
FOUND 35 ERROR(S) AND 4 WARNING(S) AFFECTING 37 LINE(S)
--------------------------------------------------------------------------------
   6 | ERROR   | Whitespace found at end of line
  10 | ERROR   | Line indented incorrectly; expected 0 spaces, found 1
  11 | ERROR   | Whitespace found at end of line
  12 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
  18 | ERROR   | Whitespace found at end of line
  34 | WARNING | A comma should follow the last multiline array item. Found: )
  44 | WARNING | A comma should follow the last multiline array item. Found: )
  68 | WARNING | Line exceeds 80 characters; contains 147 characters
 135 | ERROR   | Whitespace found at end of line
 143 | ERROR   | Whitespace found at end of line
 175 | ERROR   | Whitespace found at end of line
 179 | ERROR   | You must use "/**" style comments for a function comment
 179 | ERROR   | Line indented incorrectly; expected 0 spaces, found 1
 180 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
 181 | ERROR   | Line indented incorrectly; expected 2 spaces, found 3
 184 | ERROR   | Whitespace found at end of line
 185 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
 186 | ERROR   | Line indented incorrectly; expected 2 spaces, found 3
 189 | ERROR   | Line indented incorrectly; expected 2 spaces, found 3
 193 | ERROR   | You must use "/**" style comments for a function comment
 193 | ERROR   | Line indented incorrectly; expected 0 spaces, found 1
 194 | ERROR   | Whitespace found at end of line
 195 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
 201 | ERROR   | No space before comment text; expected "// 'deepSearch' =>
     |         | (bool) $form['script_settings']['deep_search']['#value']," but
     |         | found "//'deepSearch' => (bool)
     |         | $form['script_settings']['deep_search']['#value'],"
 205 | ERROR   | Whitespace found at end of line
 206 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
 211 | ERROR   | Whitespace found at end of line
 212 | WARNING | Line exceeds 80 characters; contains 89 characters
 213 | ERROR   | Line indented incorrectly; expected 2 spaces, found 3
 216 | ERROR   | Whitespace found at end of line
 217 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
 219 | ERROR   | Whitespace found at end of line
 220 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
 224 | ERROR   | Whitespace found at end of line
 225 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
 227 | ERROR   | Whitespace found at end of line
 228 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
 230 | ERROR   | Whitespace found at end of line
 231 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
--------------------------------------------------------------------------------


FILE: ...view/sites/all/modules/pareview_temp/test_candidate/queryloader.install
--------------------------------------------------------------------------------
FOUND 6 ERROR(S) AFFECTING 6 LINE(S)
--------------------------------------------------------------------------------
  9 | ERROR | Whitespace found at end of line
 10 | ERROR | Inline comments must end in  full-stops, exclamation marks, or
    |       | question marks
 12 | ERROR | Whitespace found at end of line
 13 | ERROR | Whitespace found at end of line
 24 | ERROR | Whitespace found at end of line
 25 | ERROR | Whitespace found at end of line
--------------------------------------------------------------------------------


FILE: ...eview/sites/all/modules/pareview_temp/test_candidate/queryloader.module
--------------------------------------------------------------------------------
FOUND 22 ERROR(S) AND 1 WARNING(S) AFFECTING 23 LINE(S)
--------------------------------------------------------------------------------
  17 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
  34 | ERROR   | Whitespace found at end of line
  38 | ERROR   | No key specified for array entry; first entry specifies key
  41 | ERROR   | Whitespace found at end of line
  67 | ERROR   | Whitespace found at end of line
  68 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
  71 | ERROR   | Whitespace found at end of line
  72 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
  74 | ERROR   | Whitespace found at end of line
  77 | ERROR   | Whitespace found at end of line
  78 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
  81 | ERROR   | Whitespace found at end of line
  82 | ERROR   | Whitespace found at end of line
  89 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
  91 | ERROR   | Whitespace found at end of line
  92 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
  94 | ERROR   | Whitespace found at end of line
  98 | ERROR   | Whitespace found at end of line
  99 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
 103 | ERROR   | Whitespace found at end of line
 107 | ERROR   | Whitespace found at end of line
 108 | WARNING | Line exceeds 80 characters; contains 86 characters
 112 | ERROR   | Whitespace found at end of line
--------------------------------------------------------------------------------

Lines in README.txt should not exceed 80 characters, see the guidelines for in-project documentation.
Remove LICENSE.txt, it will be added by drupal.org packaging automatically.
queryloader.module in queryloader.info: It's only necessary to declare files[] if they declare a class or interface.
queryloader.install in queryloader.info: It's only necessary to declare files[] if they declare a class or interface.
./queryloader.module: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
```
function queryLoader_attach($script_settings = NULL) {
```

All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting

./queryloader.admin.inc ./lib/jquery.queryloader2.js ./queryloader.install ./queryloader.info

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Source: http://ventral.org/pareview - PAReview.sh online service

Comment #2

patrickd commented 8 December 2011 at 08:14

Status:

Needs work

» Needs review

Switched back to needs review, so in-depth reviews won't be blocked by coding standart issues.

Comment #3

codesidekick commented 10 December 2011 at 23:43

Hi there,

Thanks for your hard work. I've now fixed up all the problems shown except for 38 | ERROR | No key specified for array entry; first entry specifies key
. I believe this is in relation to the following code:

$libraries['queryloader'] = array(
    'title' => 'QueryLoader2',
    'website' => 'https://github.com/Gaya/QueryLoader2',
    'version' => '2',
    'js' => array(
      drupal_get_path('module', 'queryloader') . '/lib/jquery.queryloader2.js' => array(
        'type' => 'file',
        'scope' => 'header',
        'group' => JS_LIBRARY,
      ),
      drupal_get_path('module', 'queryloader') . '/queryloader_drupal.js' => array(
        'type' => 'file',
        'scope' => 'header',
        'group' => JS_DEFAULT,
      ),
      array(
        'type' => 'setting',
        'data' => array(
          'queryloader' => variable_get('queryloader_options'),
        ),
      ),
    ),
  );

Seems to be referring in particular to the setting array in the #js property of drupal_add_library. Is there a better way of including the settings for this library? It seems pretty solid in my time testing it and there aren't any PHP validation errors that I've spotted relating to this code.

I've now also switched the GIT branch to 7-x.1-x although the project currently has two info files because the sandboxed project title isn't the intended namespace for the project.

Thanks so much for your help, hopefully I'll be able to submit this module soon and some people in the community can benefit from it.

Comment #4

patrickd commented 11 December 2011 at 00:14

You don't have to hold the old .info file, just delete it it makes no difference.

I think you can ignore the error with non specified key, it looks clear enough to me, but to be honest I've not worked with libraries yet :(

Comment #5

codesidekick commented 11 December 2011 at 11:36

Sorted out the info file. Look forward to any other reviewers out there having a look at my module. The author of the original jQuery plugin and I are pretty keen to get this module released into the wild :)

Comment #6

codesidekick commented 20 December 2011 at 22:34

Hi, I know it's Christmas time but if anybody has some time over the holidays or early next year to review this and get it to a launch phase I'd really appreciate it.

Comment #7

klausi

he/him||they/them

German

🇦🇹 Vienna

commented 20 January 2012 at 16:43

Status:

Needs review

» Needs work

There are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732

Not sure which branch I should review, 7.x-1.0 or 7.x-1.x? Please put your most recent code into the 7.x-1.x branch and remove the 7.x-1.0 branch.

Get a review bonus and we will come back to your application sooner.

Comment #8

misc commented 20 January 2012 at 17:01

About including 3rd party libraries, you should read about it here: http://drupal.org/node/422996

Comment #9

misc commented 26 May 2012 at 13:36

@martysnow has been contacted to ask if the application is abandoned.

After ten weeks with a status of needs work: the applicant may be contacted by a reviewer to determine whether the application was indeed abandoned. The action taken by the reviewer should be documented in the project application issue.

http://drupal.org/node/894256

Comment #10

codesidekick commented 6 June 2012 at 12:50

Status:

Needs work

» Postponed

Hi, I don't currently have the time to work on this but if I pick it up again I'll re-open this thread.

Thanks,

Comment #11

misc commented 6 June 2012 at 12:59

You are welcome back when you have the time.

Comment #12

klausi

he/him||they/them

German

🇦🇹 Vienna

commented 1 November 2012 at 11:38

Status:

Postponed

» Closed (won't fix)

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

QueryLoader2 integration

Comments

Comment #1

Comment #2

Comment #3

Comment #4

Comment #5

Comment #6

Comment #7

Comment #8

Comment #9

Comment #10

Comment #11

Comment #12

News items

Our community

Documentation

Drupal code base

Governance of community