Commerce add to cart popup

Alessandro,

Before an actual review, I just wanted to add a comment ... to a reviewer, the small size of this module does not provide a lot of information on which we can base an assessment of your coding style and knowledge of Drupal's APIs. As a result, upon a successful review, I would be inclined to recommend approval of the module, but with a request that you re-apply with another module before being granted the 'create full projects' permission.

Note that this is just my personal opinion, and may not be shared by other reviewers and the greater Drupal community. Also, this is not meant as any indication or criticism of your coding abilities ... rather, it's an observation that I don't really have enough information available to provide you a fair assessment.

I am surprised, as I would have assumed this functionality would already be available within the Commerce module ... but as I have never used the Commerce module myself, I can't really comment on this for sure. From this perspective, I would be interested in your thoughts regarding this being supplied as a documentation code snippet or patch for Commerce, as opposed to as a standalone module.

That said ... on with the review!

Missing README.txt

Please take a moment to add a README.txt file, following the guidelines for in-project documentation.

Master Branch

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.

License

Please remove the LICENSE.txt file. Drupal will add the appropriate version automatically during packaging so your repository should not include it.

files[] without classes or interfaces

The files[] line should be removed. It's only necessary to declare files[] if they declare a class or interface.

coder on minor

I noticed some minor Drupal code style issues (re: indentation). Please run the Coder module on "minor" setting to help catch these. The coding standards have even more information in this area.

Please note: The Coder module currently has an unresolved flaw which will prompt you to add file declarations to your .info file even when it's not necessary to do so. Please do not try to make this warning go away by declaring files which do not contain classes or interfaces.

commerce_add2cart_popup_form_alter()

Please look at the FormAPI's '#attached' property, which would be a more efficient approach for adding your library/javascript to the form.

commerce_add2cart_popup.js

Your javascript should use the Drupal behaviours model, rather than document.ready. Also, it should be wrapped in an outer context, in order to avoid namespace conflicts ... please review the information at http://drupal.org/node/756722 .

project page

please review tips for a great project page. It needs some more fleshing out so that people looking into the module might see an attached screenshot, or similar. I know it's a small module, but that means it needs to be even more descriptive about what it does.

master branch

Please clean out the master branch of old code if you are no longer using it: Moving from a master to a major version branch

removing remote branches

the method for removing the master completely (not recommended) works for any remote branch

indenting

even javascript indenting is two spaces instead of tabs

missing newlines at end of file

All text files should end in a single newline (\n). This avoids the verbose "\ No newline at end of file" patch warning and makes patches easier to read since it's clearer what is being changed when lines are added to the end of a file. See Coding standards

commenting

You only have one file and one function, but it's not commented with a function or file comment block.

wow .. so small!

I have to agree with jthorson regarding separating out full project access from the promotion of this module.

Do you venture onto IRC at all? If we get the timing right (people are awake), we can get this single project promoted into a full project, and then when you've got another release ready project, reference this thread and we can pick it up from there. Managing this project as a full project would be better than keeping it as a sandbox.

Please leave on 'active' until we get this promoted.

Sorry with all this confusion.

No problems ... the confusion is all from our side, as we try to work out a new process for promoting an individual project straight out of the review queue. Normally, we would leave the timing of the actual promotion up to the module author; which won't work if the author hasn't yet been granted the appropriate permissions. :)

On another topic, have you considered working with the commerce maintainers, and investigating the possibility of merging this functionality into the commerce module itself? It would ultimately be up to them whether they want to include it, but it doesn't hurt to initiate a conversation to judge their interest.

You may also consider merging it with Commerce add to carts extras or Commerce Extra, which would be a very sensible approach for minor functions like this.

Marking as fixed since this seems to now be included in one of the extras modules.