Text Selection

welcome,

Good to see you could already fix all coding style issues ;)

Your project page and readme is a little short, you may have a look at..
tips for a great project page.
guidelines for in-project documentation.

We do really need more hands in the application queue and highly recommend to get a review bonus so we can come back to your application sooner.

regards

As patrickd said, you should improve the README.

In addition, make people want to use your module. Tell them how it would help them.

You said you are going to make a screencast. This is a big plus!

Some mistakes I found:
blank lines on text_selection.info at lines 12, and 22
and on text_selection.variable.inc at line 19

So far, code looks good.
Later I will test it installing on Drupal.

Hello,

Ventral (http://ventral.org/pareview/httpgitdrupalorgsandboxrevagomes1552788git-7...) says

FILE: ...ew/sites/all/modules/pareview_temp/test_candidate/text_selection.module
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
14 | ERROR | Do not use t() in hook_menu()
15 | ERROR | Do not use t() in hook_menu()
--------------------------------------------------------------------------------

1. Maybe add package = User interface to the .info file?
2. When I click on configure (on the modules page) I end up on http://localhost/drupalclean/#overlay=admin/config/content
3. http://localhost/drupalclean/#overlay=admin/config/user-interface/text-s... appears to be the actual URL
4. The regular expression in the form validator allows for the # to be optional, but if you don't use it then the code doesn't add one in meaning the CSS style fails

Hope this helps.

Hi

manual review:-

• Do not use t() in hook_menu(). Because menu title are automatically t() :ed
• In function text_selection_preprocess_html(&$variables, $hook) {}, there are variable_get('text_selection_bg_color', '') and variable_get('text_selection_font_color', ''). There should be a default value for these variable, don't leave them empty.

Hi,

Some more reviews regarding coding standards.
The lines in the code should not exceed margin line of 80 characters: http://drupal.org/coding-standards#linelength.

Hello revagomes,

I am a fellow applicant and have been reviewing your project towards geeting a review bonus for my application.

Automatic review:

• ventral PAreview comes up clean

Manual review:

• Module downloads and installs OK.
• Code looks pretty clean and easily readable to me.

Food for thought:

• you may take a look at the jQuery colorpicker module, that allows to pick a color through a jQuery widget, and implement it in your configuration form - so that if that module is enabled, then it will use the specific jquery_colorpicker form element instead of the textfield.

Overall, IMHO this looks RTBC.

Regards

mondrake

Hi,
There are some typos in the README.txt
Line #23: CONFIGURATION has been misspelled.
Line #31: Interface has been misspelled.

In the text_selection.admin.inc. Line #41 to Line #52.

function text_selection_settings_form_validate($form, &$form_state) {
  if (!empty($form_state['values']['text_selection_bg_color'])) {
    $bg_color = $form_state['values']['text_selection_bg_color'];
    if (!preg_match("/^(#)([0-9a-fA-F]{3})([0-9a-fA-F]{3})?$/", $bg_color)) {
      form_set_error('error', t('The background color must fit the hexadecimal HTML color default format.'));
    }
  }
  if (!empty($form_state['values']['text_selection_font_color'])) {
    $font_color = $form_state['values']['text_selection_font_color'];
    if (!preg_match("/^(#)([0-9a-fA-F]{3})([0-9a-fA-F]{3})?$/", $font_color)) {
      form_set_error('error', t('The font color must fit the hexadecimal HTML color default format.'));
    }
  }
}

Please read the documentation of form_set_error.. You misunderstood the $name parameter of form_set_error.
The correct function should be as follows. The word 'error' should be replaced by the name of the form element. As in this case the corresponding form element is highlighted in red color to indicate error whereas in case you set it to 'error' no such highlighting should be shown in case of error. So change the function to as follows.

function text_selection_settings_form_validate($form, &$form_state) {
  if (!empty($form_state['values']['text_selection_bg_color'])) {
    $bg_color = $form_state['values']['text_selection_bg_color'];
    if (!preg_match("/^(#)([0-9a-fA-F]{3})([0-9a-fA-F]{3})?$/", $bg_color)) {
      form_set_error('text_selection_bg_color', t('The background color must fit the hexadecimal HTML color default format.'));
    }
  }
  if (!empty($form_state['values']['text_selection_font_color'])) {
    $font_color = $form_state['values']['text_selection_font_color'];
    if (!preg_match("/^(#)([0-9a-fA-F]{3})([0-9a-fA-F]{3})?$/", $font_color)) {
      form_set_error('text_selection_font_color', t('The font color must fit the hexadecimal HTML color default format.'));
    }
  }
}

Also include a validation to handle the situation in case the text field is left empty.
In the file text_selection.variable.inc, the comment(Line #3) should end with full stop.

Changing the status. Could you make the changes mentioned in above comment.

Hi there,

still some minor coding standard issues:

FILE: ...sites/all/modules/pareview_temp/test_candidate/text_selection.admin.inc
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
48 | ERROR | Expected "else {\n"; found "else{\n"
57 | ERROR | Expected "else {\n"; found "else{\n"
--------------------------------------------------------------------------------

FILE: ...es/all/modules/pareview_temp/test_candidate/text_selection.variable.inc
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
3 | ERROR | The second line in the file doc comment must be " * @file"
4 | ERROR | The third line in the file doc comment must contain a description
| | and must not be indented
--------------------------------------------------------------------------------

And a little typo in text_selection.install: line 3: TExt > Text.

I was also wondering why you have two arguments specified in hook_preprocess_HOOK()?

function text_selection_preprocess_html(&$variables, $hook) {

It just needs &$variables. And perhaps indicate that you are implementing this specific hook in the function's comment.

Please fix these issues, but otherwise, looks RTBC to me. Nice module!

Hi there,

if you want Drupal admins to approve your project faster, get a review bonus as described here: http://drupal.org/node/1410826.
If you have manually reviewed 3 other projects, you can add a review bonus tag and your approval will go faster. Then I'm talking about days instead of weeks or months.

Congratulations: your are now a vetted git user and from now on you are allowed to create full projects.

Some minor notes:

• Maybe you can use the regular expression from color_scheme_form_validate
• The function text_selection_settings_form_submit() is not really needed here because using system_settings_form for your form handles the storage of your variables automatically.

@stBorchert: please also use the template from http://groups.drupal.org/node/184389 when approving applications:

Thanks for your contribution, revagomes!

stBorchert 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.