[D7] Headbar

1. Add direct link to your git repository (git clone ...) and detailed description of what your project does in the issue summary refer:http://drupal.org/node/1011698.
2. There is still a master branch, make sure to remove it.
3. Create a proper menu link to access the Header bar settings page as we have to call the menu(admin/config/administration/headbar) directly on the browser to access the settings page.
For doing this,change menu link from "admin/config/administration/headbar " to "admin/config/people/headbar" then header bar settings link will start appearing on configuration page under people tag.

You should also change your default branch to 7.x-1.x (currently master).

And also edit this issue description to include the new git clone command (will be helpful to new reviewers).

Suggestions:

• Select what role should see the message
• Use a renderable array for the message html & allow 3rd party themes to preprocess and override the tpl.
• Add the configure property to the info file (configure = admin/config/administration/headbar).

Questions:

• How is this better than using a custom block with javascript configurations?

Hey J2R,
Please see the automated review report here.
Manual Review:
.module file:
Suggestion : You can declare variable as
$form['headbar_text] = array(
'#title' => t('Text'),
'#type' => 'textarea',
'#description' => t('Insert text which will come in the bar'),
'#default_value' => variable_get('headbar_text', 'This text will appear on header bar'),
);
rather than creating and declaring them twice in hook_init() as headbar_text and headbar_admin_settings() as msg_text.This will reduce no of variables declared but without this also project is working fine.

Changing the status from needs work to needs review.
Remove error related to master branch by
make sure active branch(git branch) is not master then run
git branch -d master
git push origin :master

I tested the module and with one exception it worked as intended. (The close arrow doesn't work in the lastest version of Opera.)

I have the following suggestions:
1. The following internal function should be renamed with an underscore:
headbar_output --> _headbar_output
Naming internal functions with an underscore, tells other devs that this is an internal function and not a call to a hook.

2. Use Drupal.behaviors API instead of jQuery(document).ready()
change the
jQuery(document).ready(function() {

to:
Drupal.behaviors.headbar = {
attach: function (context, settings) {

3. As mentioned above, I tested it in firefox, chrome and opera. The close functionality does not
work on Opera. I do not know the policy on this, perhaps switching to Drupal.behaviors will fix this.

4. You probably want to disable the headbar on admin pages. Or at least offer that option on
the configuration page.

Thanks,
Bart

Hey j2r,

the changes look good! That's great it's working in Opera too!

With point #4, I think you could add something as simple as the following to line #36 in headbar.module:

if (strpos($_GET['q'], 'js/') === 0 || strpos($_GET['q'], 'admin/') === 0) {

I only mentioned disabling on admin pages because with the admin menu, you can't see the headbar anyway. So it would seem to make sense to disable it there so that it doesn't interfere with the admin menu.

Thanks,
Bart

some manual review.
All the admin related code should go in a differen file. In this case it would be headbar.admin.inc so the corresponding change in the implementation of hook_menu() would be.

**
 * Implements of hook_menu().
 */
function headbar_menu() {
  $items['admin/config/user-interface/headbar'] = array(
    'title' => 'Header bar settings',
    'description' => 'Change how the header bar appear and the text',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('headbar_admin_settings'),
    'access arguments' => array('administer headbar'),
    file' => 'headbar.admin.inc, // This is a new line.
    'type' => MENU_NORMAL_ITEM,
  );
  return $items;
}

And the function headbar_admin_settings() would go in the file headbar.admin.inc. All the descriptions shoudl end in a full stop.
Would be great in case you could include validation function for the textfield so the user can enter only the correct input.

Happy coding :)

I'm sorry for the delay!
There are about 100 other applications waiting for review and only a hand full of active reviewers.
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.

Added some reviewed modules link

Changed git command so it's accessible for non-maintainers.

Automatic review

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732.

Manual review

General
There are a few grammatical and typographical errors, for example the module's description makes little sense: "Create a header bar which will appear on the header of the page after extend of time".

You also haven't answered the question in comment #3: "How is this better than using a custom block with javascript configurations?"

headbar.admin.inc
Instead of _headbar_is_empty, you should set #required => TRUE on the form elements you don't want to be left empty.

You also don't need to say "Error: " at the beginning of the strings you pass to form_set_error().

I suggest renaming _headbar_is_numeric() to something like _headbar_validate_delaytime(), to show more clearly that the validation function is specific to that element.

README.txt
This says the configuration page is at admin/config/administration/headbar, but it's actually at admin/config/user-interface/headbar.

headbar.module
_headbar_output() should make use of the Drupal Theme API, probably as a template file for a custom theme hook. See docs at http://drupal.org/node/933976.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

Changed git command so 7.x-1.x branch is selected...