[D7] Brow.si

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

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.

Hi mysiteapp,

I did a short review of your module and found a few minor issues. But here are a few suggestions for improvements to your module:

1. The path to your configuration defined in the info file is wrong. Your info file says the configure path is "admin/config/browsi/settings", but the correct path should be "admin/config/system/browsi".
2. Consider moving the browsi_admin() function into its own admin file - browse.admin.inc. Its much cleaner to maintain all admin related functions in an admin include file.
3. Use the hook_page_alter() function to add your javascript code to all your pages. Your drupal_add_js() function should then be declared inside this hook_page_alter() function.
4. What is the purpose of the constant BROWSI_ADMIN_URL? I dont see it being used anywhere.

Otherwise no major flaws. Good luck!

Hi mysiteapp,

I just see one small thing in browsi.admin.inc

/**
 * Implements hook_admin().
 */
function browsi_admin() {
  // Code
}

I don't think we have hook_admin() in Drupal? Yes?

Hi @mysiteapp,

I have just done a quick review and spotted a couple of things.

1. In browsi.module
- Line 31: I am not sure if you need the "return NULL" there?!
- Lines 37 to 50: I think you could use a few blank lines to separate the code and improve readability.

And for all your text files, you should include a single new line at the end. "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". Check out the coding standards https://drupal.org/coding-standards

Cheers

Hi mysiteapp, sorry for the late reply

Here is my review

Automated tool: there're some warnings
http://ventral.org/pareview/httpgitdrupalorgsandboxmysiteapp1992988git

Manual:
In browsi.admin.inc, I see that browsi_admin_validate() isn't called anywhere. Perhaps you might want to change it name to browsi_admin_settings_validate() or use #element_validate to validate the key length

Everything seems to be RTBC to me, you just need to fix those issues.

Hi all

Change status issue.

Sorry, just one small thing in browsi.admin.inc

      t("An unique identifier for this site as defined in <a href=\"!browsi_dashboard\" target=\"_blank\">Brow.si dashboard</a>. If left empty, analytics data won't be collected for this site.", array('!browsi_dashboard' => 'https://brow.si/dashboard')),

You should use "@browsi_dashboard" instead of "!browsi_dashboard". Because the link can contain special characters in the future. You can take a look at the document here https://drupal.org/node/322774

Anyway. This looks RTBC to me

Thank you for your effort. In order to get review from the admins, you need a review bonus https://drupal.org/node/1975228

PAReview: Individual user account

It seems you are using a non-individual account.
All user accounts are for individuals. Accounts created for more than one user or those using anonymous mail services will be blocked when discovered (see Get a Drupal.org account).
Please change your account information and enter your realname.

* You are using hook_perm() in your Drupal 7 module file. This won't work because the hook is now named hook_permission().

Your project page is here: https://drupal.org/sandbox/mysiteapp/1992988.

This review is for the 7.x branch only.

Can you update the project page?

Brow.si is a groundbreaking user engagement Drupal module for the mobile web. It provides web publishers with powerful extensions that significantly enhance the user experience and engagement with respect to their sites and content on mobile devices.

doesn't really tell me anything, it's all buzzwords. What does the module actually do?

Many modules that use js libraries offer an option to dl the library locally and use that instead of calling out to an external provider. Not only is this more reliable, but it allows Drupal's js performance aggregation to activate as well. That's just an idea.

Seems reasonable enough, those issues are minor.

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

I still don't like the .info description Your mobile browsing is about to be... Improved! but that's not so important :)

Thanks for your contribution, kobim14!

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 - Crafted, Curated, Contributed.