Viewport

Hi Thorin, overall your module looks very good.

Handy when implementing responsive designs!

Some little issues I saw when reviewing:
- no help text in viewport_help. You might refer to some external website with more info about viewports.
- no validation of any of the variables. This may be a design decision, but why not checking values such as viewport_initial_scale, viewport_minimum_scale, etc? You can make your interface more user-friendly in that way, and decrease the chance of errors. At least check for the existence of comma's, as it seems that it would break the meta tag content in viewport_preprocess_html in viewport.module. Even though this is unlikely to happen assumptions are usually bad.
- A tiny grammatical thingy: 'the same value than the' should be 'the same value as the'. Thumbs up for some good descriptions of the values to be set overall. In general your language and descriptions are clear and helpful.
- In viewport.module line #87, use current_path instead of $_GET['q']. This wrapper function does the same, but is there to decrease coupling and should be used.

Regards,
Bas

One more thing Thorin, can you make a separate branch for the drupal 7 version? So that your branch is not 'master' but '7.x-1.x'?

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.
Review of the master branch:

• README.txt is missing, see the guidelines for in-project documentation.
• Run coder to check your style, some issues were found (please check the Drupal coding standards). See attachment.
• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

Thanks to you thorin_edr :)

I found some minor issues in the code.

There are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732. In other words your master branch should be empty.

Please check attachment for Coding Standards or http://ventral.org/pareview/httpgitdrupalorgsandboxslv1490000git-7x-1x.

My personal observations:

• Short description for the viewport_path_is_selected() is missing in the viewport.module line 82.
• Viewport.module line 83 and 87: Data type is not necessary, please check Doxygen and comment formatting conventions: Documenting functions and methods.
• Add full-stop in @file comment descriptions. please check Doxygen and comment formatting conventions: Document Files.

¡Vamos amigo falta poco!

Hello @thorind_edr:

In my manuel review I found minor coding issues on viewport.admin.inc, for more please use http://ventral.org/pareview to check and fix it.

Hi,

You are already using t() function here.

$path_patterns_description = t("The viewport settings will be applied to the following paths. <br/>
Specify pages by using their paths. Enter one path per line. The '*' character is a wildcard.
Example paths are %blog for the blog page and %blog-wildcard for every personal blog.
%front is the front page.", array(
'%blog' => 'blog',
'%blog-wildcard' => 'blog/*',
'%front' => '<front>',
));

You can remove
t() function from line #78
'#description' => t($path_patterns_description),

You have not listed any reviews of other project applications in your issue summary as strongly recommended here: http://drupal.org/node/1011698

If I checkout http://git.drupal.org/sandbox/slv/1490000.git, my local copy is empty.

I just did another manual & automatic review of this module.

The automatic review showed two 'criticals' but these proved to be false positives after manual review. These can be prevented by moving the value of $path_patterns_description to $form['viewport']['viewport_selected_pages']['#description'] directly. I would think this is a good thing to do anyway, as the descriptive text is now somewhere in the top of the function; it is more logical to put it right where it's necessary only. However, this is a tiny issue.

Overall I think all the correct fixes have been applied. Code is safe, well documented and clean, and the user showed that he has the appropriate knowledge to build modules with good code quality.

Now it's up to someone who can to give this user the 'create full projects' permission.

You have not listed any reviews of other project applications in your issue summary as strongly recommended in the application documentation.

manual review of the 7.x-1.x branch:

1. "t($error_message, ...": do not pass dynamic variables to t() where possible, as this cannot be picked up by translation extraction tools. Same in viewport_help().
2. viewport.info: "description" should be on one line.
3. viewport_preprocess_html(): the "else" branch is useless, so remove it.

But that are just minor issues, so ...

Thanks for your contribution, thorin_edr! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.