Field Group List

I really can't find any major issues with this module. Perhaps just a thought: you could implement your own theme function so users could change the markup of the module.

Cheers.

I guess users can "getting rid of the basic markup" in a lot of places across Drupal, but that makes it flexible. Although your idea is also nice. Perhaps you could implement both: more "advanced" users could use the theme override (perhaps add a wrapper), those who want to build their sites thought the UI could fill in the form.

Cheers.

That was fast :)

So here are some thoughts:

Probably theme_field_group_list_group_wrapper should be enough. There is no need for theme_field_group_list_field_wrapper. I think you can safely print li-s inside theme_field_group_list_group_wrapper.

You are using #prefix, #suffix inside your theme functions. I think that something like this:

 if ($group->format_settings['formatter'] == 'ordered') {
   $output = '<ol class="field-group-format ' . $group->group_name . ' ' . $classes . '">' .  /* elements */ . '</ol>';
 }

is more common practice in Drupal. Perhaps you should get a second opinion on that.

I am not sure if you need a theme function for the help. I looked at some core implementations of hook_help and none of them implements a theme function for the help message and they have far more complex help than yours.

Cheers.

Like this:

  foreach (element_children($element) as $field_name) {
    $items[] = '<li class="' . $group->group_name . ' ' . $classes . '">' . drupal_render($element[$field_name]) . '</li>';
  }

  if ($group->format_settings['formatter'] == 'ordered') {
    $element = '<ol class="field-group-format ' . $group->group_name . ' ' . $classes . '">' . implode("\n" , $items) . '</ol>';
  }
  else {
    $element = '<ul class="field-group-format ' . $group->group_name . ' ' . $classes . '">' . implode("\n" , $items) . '</ul>';
  }

You would then have to change field_group_list_field_group_pre_render into something like this:

  if ($group->format_type === 'list') {
    $element['#theme'] = 'field_group_list_group_wrapper';
    $element['#element'] = $element;
    $element['#group'] = $group;
  }

But like I said, perhaps you should get a second opinion on that. I was just trying to look to this theme function like a themer.

I've followed instructions on your project page and there is one more thing probably should mention.
Many users may choose to enable your module after they added all the fields and fieldsets. Sometimes they add fieldsets not on Manage Display page, but on Manage Fields Page. There is no option on Manage Fields to add your type of Fieldset. What's more - if user wants to have the same structure he or she should clone fieldsets on Manage Display page rather than add them again.
I think it's worth mentioning on the project page and it will be good if I could add List display on Manage Fields (so they will show on entity add / edit screen also).

I do not understand why help page has it's own .tpl.php file. It allows themers to change help page structure and texts. As was said even more complex modules do not do that.

On your tpl.php file you have:
print t('About Fields Group List')
You should always place semicolon at the end of the line, even if you use ?> on the same line.

In your comments you should start with a capital letter and end with a full stop (for example in @file block).

I believe these are not blockers.. Your module does what you said, I have not found security issues (although I'm pretty new in reviewing project applications) so I believe it is RTBC.

In the meantime:
http://ventral.org/pareview/httpgitdrupalorgsandboxsirviejo1710684git
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

Hi,

As I'm not Git Administrator I cannot promote your module to full project. The next step is waiting for one of Git Administrators to approve your module or set it once again to needs work if they find something wrong.
I believe you can quicken this process by Review bonus.

Other helpful resource is http://drupal.org/node/539608.

Good luck :)

This might be suitable to go directly into field_group, right? Please open an issue in the field_group queue to discuss that and get in contact with the maintainer(s). We prefer collaboration over fragmentation to limit the search hassle for site builders, so maybe this is suitable to be in one module. Please get back to us if that is not appropriate or close this issue if it was successful.

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

typo