This is a Drupal 7 implementation of the BBC news team's Imager.js.
https://github.com/BBC-News/Imager.js/

Imager.js is an alternative solution to the issue of how to handle responsive
image loading, created by developers at BBC News.

It provides a field formatter that will take fields attached to a node and use Image Styles to create size variations on the original image. It will then pass these image styles to an instance of Imager.js.

https://www.drupal.org/sandbox/tomjamesallen/2572263

Git clone command:
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/tomjamesallen/2572263.git imager_js
cd imager_js

CommentFileSizeAuthor
#7 imager_js_pareview.txt5.59 KBmelvix

Comments

tomjamesallen created an issue. See original summary.

tomjamesallenweb@gmail.com’s picture

tomjamesallenweb@gmail.com commented
Title: [D7] imager_js » [D7] Imager.js
beniamin.szabo’s picture

beniamin.szabo
Primary language Romanian
commented
Status: Needs review » Needs work

The automated review returned some issues. Please fix them.

tomjamesallenweb@gmail.com’s picture

tomjamesallenweb@gmail.com commented
Status: Needs work » Needs review

Hi Beniamin,

Thank you for your time. I did an automated review prior to submitting the project and eliminated the majority of the errors.

I don't believe any of the remaining errors are problems with the code. I have outlined them below, please let me know if you have any feedback.

Kind regards,

Tom

Eslint JS errors

The following JS errors are caused by the semi-colon immidiately before the opening function. ;(function ($, window) { This is a best practice for JavaScript plugins, as it avoids errors in the case that the previous JavaScript is missing the semi-colon on the last line.

js/Imager.useMultipleSrcs.js: line 7, col 1, Error - Unnecessary semicolon. (no-extra-semi)
js/imager-js-setup.js: line 7, col 1, Error - Unnecessary semicolon. (no-extra-semi)

The third JavaScript error is caused be a console.log(). I believe this is an acceptable use of console.log(), as it is there for the benefit of the developer, and notifies them if the required JS library (Imager.js) is missing. This output should never appear in production code providing the module has been correctly installed.
js/imager-js-setup.js: line 18, col 7, Error - Unexpected console statement. (no-console)

DrupalPractice

The following errors are caused by field option labels not being passed through the translate function. In these cases, the field label is not an item that should be translated. This option lets the user select which html tag should be used as a wrapper for the image. On line 72 the label is '<div/>', and line 73 '<span/>'. 

FILE: ...eview/pareview_temp/includes/create_field_formatter_settings_form.inc
---------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
---------------------------------------------------------------------------
72 | WARNING | #options values usually have to run through t() for
| | translation
73 | WARNING | #options values usually have to run through t() for
| | translation

For the following error, drupal_add_js() and drupal_add_css() seem like the best options. According to this thread the method recommended below isn't 100% reliable, and drupal_add_js() and drupal_add_css() are perfectly acceptable.

FILE: /var/www/drupal-7-pareview/pareview_temp/imager_js.module
---------------------------------------------------------------------------
FOUND 0 ERRORS AND 4 WARNINGS AFFECTING 4 LINES
---------------------------------------------------------------------------
76 | WARNING | Do not use drupal_add_js() in hook_page_build(), use
| | #attached on the $page render array instead
79 | WARNING | Do not use drupal_add_js() in hook_page_build(), use
| | #attached on the $page render array instead
91 | WARNING | Do not use drupal_add_js() in hook_page_build(), use
| | #attached on the $page render array instead
94 | WARNING | Do not use drupal_add_css() in hook_page_build(), use
| | #attached on the $page render array instead

Finally, the last two errors for the JS files are referring to the same semi-colons as the first two JS errors.

PA robot’s picture

PA robot commented
Status: Needs review » Needs work

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

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.

tomjamesallenweb@gmail.com’s picture

tomjamesallenweb@gmail.com commented
Status: Needs work » Needs review

I have already gone through the results from the automated test. Please see above.

melvix’s picture

melvix commented
Status: Needs review » Needs work
StatusFileSize
new imager_js_pareview.txt5.59 KB

Automated Review

See attached file.

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
Yes: Does not cause module duplication and/or fragmentation.
Master Branch
Yes: Follows the guidelines for master branch.
Licensing
Yes: Follows the licensing requirements.
3rd party assets/code
Yes: Follows the guidelines for 3rd party assets/code.
README.txt/README.md
No: Does not follow the guidelines for in-project documentation and/or the README Template.
Module requirements listed in .info are not documented in README.
Code long/complex enough for review
Follows the guidelines for project length and complexity.
Coding style & Drupal API usage
[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:
  1. (*) Throughout imager_js.module, imager_js_include() is called outside any function scope. Since it only includes files, those files should be probably be added in the .info file: files[] = myinclude.inc.
  2. (*) Hook_permission() creates permission administer_imager_js, but it isn't referenced or used anywhere else.
  3. (*) The field format options includes a 'JS Plugin' option that is not referenced in the README. It is a selector with one option 'Please install a js plugin' with no other directions.
  4. (*) After saving an image field using Imager.js as the format, the image disappeared from the full content display.
  5. The README is difficult to read. Particularly this:
          This project uses Drupal's libraries module to handle loading the Imager.js
project. You will need to download the full Imager.js repository from github
and place it in your libraries directory. `sites/all/libraries` or 
`sites/< domain >/libraries`. This directory is automatically created by the 
libraries module.

The outer directory containing the Imager.js project should be named
`Imager.js`, if you download the zip from the project page you may have to
rename the directory from `Imager.js-master` or similar. You should end up with
the file `Imager.min.js` located at the path 
`[libraries-path]/Imager.js/dist/Imager.min.js`. If the minified file is not
found, the module will also attempt to use the non-minified file located at 
`[libraries-path]/Imager.js/Imager.js`.

    Suggest that you look at some existing 3rd-party integration modules to see how they handle their download/unpack/rename instructions.

  6. It would be nice if the module checked for the presence of the Imager library and displayed an error when it isn't found.

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.

This review uses the Project Application Review Template.

PA robot’s picture

PA robot commented
Status: Needs work » Closed (won't fix)

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.