Hi,

As mentioned in my e-mail this is a module I've created for flipping the rows and columns of a view. Most of the code is in the file views-flipped-table.tpl.php and is just copied from http://drupal.org/node/174578. The other files turn it into a views style plugin module.

Some more info at http://groups.drupal.org/node/61223.

What I need now is a general review of the code, since I'm not an experienced php/module-developer. I also have some specific issues that I want to be reviewed in particular:

1. Should the file theme.inc be called something else? Not sure about the naming convention for that.
2. What's the line " * @ingroup views_templates" doing in views-flipped-table.tpl.php? Should it be removed or changed to something else?
3. Please look through the arguments in views_flipped_table.views.inc. I'm not sure what arguments like 'uses row plugin' and 'uses fields' are for.

Feel free to add this module to Views Hacks if you find that appropriate. Otherwise I can go through the cvs access process and submit it as a separate module with myself as the maintainer.

Thanks,
Adam

CommentFileSizeAuthor
#4 views_flipped_table_2.zip2.08 KBnadam
#1 views_flipped_table.zip2.27 KBnadam

Comments

nadam’s picture

nadam commented
StatusFileSize
new views_flipped_table.zip2.27 KB

Adding the attachment again.

OT: Seems to be a bug when doing things in the following order:

1. Attaching a file
2. Previewing the issue
3. Saving

I've seen the same thing happen on g.d.o.

/Adam

fabsor’s picture

fabsor commented
Status: Needs review » Needs work

Hi!

Some thoughts when looking at the code:

views_flipped_table.views.inc is missing a comment with an @file annotation at the top of the file.
theme.inc is missing a comment with an @file annotation at the top of the file.

views_flipped_table.views.inc, line 6:

require_once "./$path/theme.inc";

You could probably use module_load_include instead. Is it even necessary to include it here?

and line 20:

'theme path' => "$path",

no use for "" here.

Otherwise the code seems fine to me! good work.

fabsor’s picture

fabsor commented

Another comment after a little testing:

theme.inc is probably unnecessary for this module since it only contains one function and adds the extra work of having to include it. why not just include the function in your .module file, and change your style declaration to something like this?

  return array(
    'style' => array(
      'flipped' => array(
        'title' => t('Flipped table'),
        'help' => t('Displays a table with rows and columns flipped.'),
        'handler' => 'views_plugin_style_table',
        'parent' => 'table',
        'uses row plugin' => FALSE,
        'uses fields' => TRUE,
        'uses options' => TRUE,
        'type' => 'normal',
        'theme' => 'views_flipped_table',
        'even empty' => TRUE,
      ),
    ),
nadam’s picture

nadam commented
Status: Needs work » Needs review
StatusFileSize
new views_flipped_table_2.zip2.08 KB

Thanks fabsor. All your comments made sense. I have now updated the code and removed the themes.inc file.

infojunkie’s picture

infojunkie
Location Vancouver
commented
Status: Needs review » Fixed

Committed to the latest dev. Thanks to the contributor and reviewer!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

BenK’s picture

BenK commented
Component: Code » General

Keeping track of this thread... any plans for Drupal 7?