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.

CommentFileSizeAuthor
#7 713750-theme_views_slideshow_main_section4.patch40.77 KBpsynaptic
#3 713750-theme_views_slideshow_main_section3.patch40.78 KBpsynaptic
#2 713750-theme_views_slideshow_main_section2.patch1.22 KBpsynaptic
#1 713750-theme_views_slideshow_main_section.patch2.66 KBpsynaptic

Comments

psynaptic’s picture

psynaptic commented
Status: Active » Needs review
StatusFileSize
new 713750-theme_views_slideshow_main_section.patch2.66 KB

Patch attached.

psynaptic’s picture

psynaptic commented
StatusFileSize
new 713750-theme_views_slideshow_main_section2.patch1.22 KB

Sorry, last patch was contaminated.

Here's another clean one.

psynaptic’s picture

psynaptic commented
StatusFileSize
new 713750-theme_views_slideshow_main_section3.patch40.78 KB

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

psynaptic’s picture

psynaptic commented
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.

psynaptic’s picture

psynaptic commented
Title: Reformatted code structure for coding standards and usability » Reformatted code structure for coding standards and readability
Standart’s picture

Standart commented

"Implementats hook_help()."

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

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

psynaptic’s picture

psynaptic commented
StatusFileSize
new 713750-theme_views_slideshow_main_section4.patch40.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?

Standart’s picture

Standart commented
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...

psynaptic’s picture

psynaptic commented

The spacing around the concatenation operator used to be to not have a space on the side of the quote e.g.

$var = $var .' string';

But to follow other languages and for readability it was changed for D7.

I agree with your other comments but I have been liking simple variables embedded in strings recently, maybe I'm getting a bit obsessive.

psynaptic’s picture

psynaptic commented
Status: Needs review » Fixed

This got committed to DRUPAL-6--1. I'd love to know if there are other improvements to the code you know about.

Standart’s picture

Standart commented

Great!

You probably mean DRUPAL-6--2...

psynaptic’s picture

psynaptic commented

Good catch, DRUPAL-6--2 ;)

Status: Fixed » Closed (fixed)

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