Reformatted code structure for coding standards and readability [#713750]

Before:

function theme_views_slideshow_main_section($id, $hidden_elements, $plugin) {
  $output = "\n\n" . '<div id="views_slideshow_' . $plugin . '_main_' . $id . '" class="views_slideshow_' . $plugin . '_main views_slideshow_main">' . "\n  ";
  $output .= $hidden_elements;
  $output .= '</div><!--close views_slideshow_' . $plugin . '_main_' . $id . "-->\n\n";
  return $output;
}

After:

function theme_views_slideshow_main_section($id, $hidden_elements, $plugin) {
  $classes = array(
    "views_slideshow_${plugin}_main",
    'views_slideshow_main',
  );
  $attributes['class'] = implode(' ', $classes);
  $attributes['id'] = "views_slideshow_${plugin}_main_${id}";
  
  $attributes = drupal_attributes($attributes);

  return "<div$attributes>$hidden_elements</div>";
}

I think it's a lot cleaner and easier to work with. I'm happy to hear any ways this could be further improved.

Comment	File	Size	Author
#7	713750-theme_views_slideshow_main_section4.patch	40.77 KB	psynaptic
#7
#3	713750-theme_views_slideshow_main_section3.patch	40.78 KB	psynaptic
#3
#2	713750-theme_views_slideshow_main_section2.patch	1.22 KB	psynaptic
#2
#1	713750-theme_views_slideshow_main_section.patch	2.66 KB	psynaptic
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

psynaptic CreditAttribution: psynaptic commented 13 February 2010 at 22:55

Status:

Active

» Needs review

File	Size
713750-theme_views_slideshow_main_section.patch	2.66 KB

Patch attached.

Comment #2

psynaptic CreditAttribution: psynaptic commented 13 February 2010 at 23:16

File	Size
713750-theme_views_slideshow_main_section2.patch	1.22 KB

Sorry, last patch was contaminated.

Here's another clean one.

Comment #3

psynaptic CreditAttribution: psynaptic commented 14 February 2010 at 03:10

File	Size
713750-theme_views_slideshow_main_section3.patch	40.78 KB

I spent the night going crazy on this module. I hope you approve of the changes.

Comment #4

psynaptic CreditAttribution: psynaptic commented 14 February 2010 at 12:50

Title:

Reworked theme_views_slideshow_main_section

» Reformatted code structure for coding standards and usability

More accurate title.

BTW: I'd love to be co-maintainer if you like what you see.

Comment #5

psynaptic CreditAttribution: psynaptic commented 14 February 2010 at 19:57

Title:

Reformatted code structure for coding standards and usability

» Reformatted code structure for coding standards and readability

Comment #6

Standart CreditAttribution: Standart commented 15 February 2010 at 00:03

"Implementats hook_help()."

You may not have a space between quotes and the . (dot).

Check scripts/code-style.pl in your Drupal dir.

Comment #7

psynaptic CreditAttribution: psynaptic commented 15 February 2010 at 10:24

File	Size
713750-theme_views_slideshow_main_section4.patch	40.77 KB

You may not have a space between quotes and the . (dot).

This is the new standard for Drupal 7, it is in preparation for porting to 7.

Typo fixed.

Is this patch otherwise fine?

Comment #8

Standart CreditAttribution: Standart commented 23 February 2010 at 12:35

Category:

feature

» task

I like it!

In line 276, 280, 649, and 653: double spaces

I personally would pefer

$output = '<div' . drupal_attributes($attributes) . '>...

over

$attributes = drupal_attributes($attributes);
$output = "<div$attributes>";

In .tpl files shouldn't we do something like

<?php if ($options['thumbnailhover']['controls'] == 1): ?>
  <?php print $controls; ?>
<?php endif; ?>

instead of

if ($options['thumbnailhover']['controls'] == 1) {
  print $controls;
}

The quote-space thing is interesting. Whole Drupal 6 code base does not put a space after a quote but I couldn't find any hint about it. The coding standard only contains space-quote-space...