[D7] Amazon SendtoKindle Button Integration

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.

No issue found by PAReview. Its good to see a clean code !!

please update the project description to have non-maintainer GIT clone link

1. Issue :hook_permission have been declared incorrectly. There is a Typo, its defined as amazon_send_to_kindle_ipermission()
2. Suggestion:
   • include Configuration page link in Info file.
   • in hook_menu() its not necessary to explicitly mention system.admin.inc.
   • in line 158/159 : you can reduce the call to drupal_get_path() if the same value can be assigned to a variable & using the same.
   • if you have a variable that hold path information, then you can pass the same to _amazon_send_to_kindle_get_html() & utilize it in line 198. Reducing few function calls will improve performance on larger sites.

Please manually review 3-4 Projects & have a review bonus tag added to your application. Project applications with review bonus always get priority
Also please go thru the application submission checklist which would be of great help.
Additionally you can also check Apply for permission to create full projectspage for clear instruction on how to have your project description updated.

Once you are done with necessary changes please change the status to Needs Review so that other contributors would start looking at this project.

Hi mohit_rocks,

Thanks for submitting! The module code is simple and straightforward.

However, I think this module appears to duplicate functionality from Readability Button. There was also a separate application a ways back for this same feature, and it was closed due to duplication of Readability Button.

Could you talk a little, then, about the differences between your module and Readability Button?

I also saw another potentially important issue: the kindle.css file in your module is licensed from Amazon. It is GPL, but from what the handbook says, a drupal.org admin will have to make an exception for this to get through. You might save yourself some time if you find another way to include this css — by linking to it remotely, etc.

Thanks!

Hi Mohit,

I just wanted to add a few other notes from my manual review so that you have everything here.

1. You have a drupal_add_js() in hook_init(). While this works, it's a bit inefficient to call this on every page. I recommend moving this to your hook_node_view(), to live with your other drupal_add_js().
2. On that note, I would recommend moving your drupal_add_js()'s and drupal_add_css() into the conditional on line 162. This way the files only get called when viewing nodes of the correct type, of the correct display.
3. You're outputting the button html in _amazon_send_to_kindle_get_html(). As a themer, I would prefer that this were in a theme function — especially since you're using inline styles. Otherwise, it's difficult for developers to override your output.
4. Your js is throwing an error in the console:
   Uncaught TypeError: object is not a function
   Looks like this is because of the parentheses on line 6. The code works with and without the parentheses, but it ceases throwing an error when they're pulled out.
5. Just a recommendation for your settings page: you're currently limiting display types to Full nodes and Teasers. Why not let the button render on other (even custom) displays? I could see several use cases for that.
6. You're creating a new Social menu item under Configuration. I'm not sure what the official policy is on this, but I can see potential collisions with other modules trying to use that path. It might make more sense to put your path under Services or Content Authoring.

Thanks!

majorrobot wrote:

You might save yourself some time if you find another way to include this css — by linking to it remotely, etc.

I think linking an off-site library is a no-go because of the security implications. The guidelines say you should supply download and installation instructions.

In this case, I think it makes a lot of sense to include it in the project repo, so I suggest the maintainer ask for the required permission and only explore other options if permission is not granted.

It will be OK with me, but I am not the person that makes the final decision.

Since it is licensed under GPL, it is permitted to include it if you get permission. Why don't you ask the Drupal.org webmasters first, and only fall back on changing the css if permission is not granted?

If you do ask, mention it here. I'll chip in and give my support (but I am not the one that makes this decision).

PS: The git clone command (mentioned in #2) is still wrong. It will only work for you - not for reviewers.

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.