[D7] Turn.js

Welcome,

please get a review bonus first. Then try to fix issues raised by automated review tools: http://ventral.org/pareview/httpgitdrupalorgsandboxmichaellenahan1798746git

Hi,

I took a look at the code and it could be improved at several places:

• The php code is not up to the Drupal coding standards. Use the coder module (http://drupal.org/project/coder) to verify your code. One thing I saw was a ; after loops/functions, this is not required.
• The javascript code doesn't use Drupal behaviors. It should be changed to do so.
• The javascript code is not up to the Drupal coding standards (http://drupal.org/node/172169), some tips
  • Keys in an object don't need '', 'width': should be width:
  • Camelcased variables in the settings are cleaner so you can use the following: Drupal.settings.turnjs.pageWidth
• Relying on git for installation of the library is not friendly for a lot of users, include a direct download link or other instructions in the README.txt

In my opinion all those jQuery-Pageflip plugins are hard to handle for the end-user, cause not everybody is able to export PDFs to images in the correct sizes / correct resolutions etc.. That's why i often recommend using a flash-tool and putting the exported data on the web-server.

In most cases users just want to export an existing PDF as a page-flip for their website..

BUT - we could do better - turn.js is a really nice/slim plugin that would suite most needs if we improve the handling - based on the following content-type structure:

• Filefield to upload PDF-File
• Imagecache Preset that uses ImageMagick to convert PDF via convert with appropriate configuration in images used by turn.js(configurable)
• Autopopulate the imagefields in Content-Type with images
• Display-Formatter that uses turn.js for outputting images as page-flip"thing"

What do you think of this approach ?

Project applications are tasks.

I just had a quick read through of your code and noticed that turnjs.js line 3 refers to exampleModule - probably does not affect the actual functionality, but should probably refer to your module.

Okay, doing a code review for you!

Installing and actually making it work:

So installing the module was a bit of a challenge. Your readme code actually has TWO explenations for how to install and where to install, one telling you to clone the turn.js repo... which puts it in a folder called turn.js, and above you say to put it in turnjs (line 7). Pick one, make sure it's right, and improve the clarity on this please!

From there, I set it up, uploaded some images, and went to see how it works. This must be a turn.js issue but... The images are shifted when you mouse over the page flipcorner. It's visually off. Please review if it's your code.

Ventral code review

FILE: /var/www/drupal-7-pareview/pareview_temp/README.txt
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AND 1 WARNING(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
7 | WARNING | Line exceeds 80 characters; contains 81 characters
57 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

Manual Code Review

Nothing bad or security wise sticks out to me. It's just got some wonky functionality and the readme needs a serious overhaul for extreme clarity!

Hey Risse,

I've installed the module and went through the code. I liked the module, it's pretty light and gives and interesting functionality.
Here are my tips:

README.txt
You have a conflict in the file, please examine it and commit the correct changes.
Also I agree with jnicola regarding the installation process.

Clone the turn.js file into the sites/all/libraries/turnjs/ folder:

But your code is looking for the "turn.js" folder. I had /turnjs/turn.js/turn.js and it didn't see it.

turnjs.module
Line 53. Specify the correct comment (because you're actually implementing a hook there: hook_field_formatter_settings_form).

turnjs.js
You should specify a correct behavior name (not exampleModule).
Make a check if there's such a function named jQuery.turn() and also make a quick check for settings.turn values. Also don't be afraid to use $ instead of jQuery, because it's namespaced. Drop a line of comment in your js file and indent the lines correctly according to: JavaScript Drupal Standards.

General:
Images get always distorted, even if I use an image style. And just like jnicola wrote, I had reproduce the case of image shifting on a hover event.
Also I've noticed that when I disabled image styles, the turn.js picked the largest image and used it's width / height in the whole widget, which resulted in a huge turn.js widget (wider than my page/screen), I guess that's also an issue you have to look into.

I've noticed that first image (in double mode) is always displayed on the right side of the double layout, maybe you want to expose that into settings, so users can pick either one image is shown or both.

Good luck!

You like a lot of whitespace! In general, Drupal prefers that you don't do that. That's very minor though, and the issues above refer to possible rendering bugs, but I'm sure the module's users will let you know :) RTBC from me.

--
Top Shelf Modules - Enterprise modules from the community for the community.

Hi, I wanted to say thanks to Risse and put a note on this issue because we worked together on the original project and it's in my sandbox --- just want to make it clear that all the credit belongs to Risse, not me.

I'm going to give this a go on my local dev site. However does anybody think its possible for this module to display pages as views or furthermore views within views as a page in addition to the imagefield to PDF option? There is a module that user Luch developed that attempted to do this and sent for me to test called Booklet for views. It was based on the Moleskin Booklet using the Booklet jQuery plugin. I am not adept at Drupal module development however I am pretty good at testing. If anyone wants to give it look let me know the best way to get it to you. I'm not attempting to sidetrack the thread I just think that both modules can benefit somewhat if some ideas and code are shared.

Code still looks good to me, and it's been over a month since RTBC.

Thanks for your contribution, Risse!

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.

----
Top Shelf Modules - Enterprise modules from the community for the community.