weblinks-link.tpl.php has a bunch of issues, including not appropriately commenting Divs, not having a class="title" on the h2 and displaying divs even if there is no content in the div.

The patch is made against alpha3 and fixes all the issues.

CommentFileSizeAuthor
webform-links.tpl-041409.patch1.55 KBkmonty

Comments

kmonty’s picture

kmonty
He/Him
Location Berlin, DE + SF, CA
commented

This line should also be removed from any release:

<!--pre><?php print_r (get_defined_vars()); ?></pre-->
 <div class="weblinks weblinks-item weblinks-link<?php print $variables[0]->nid;?>">
kmonty’s picture

kmonty
He/Him
Location Berlin, DE + SF, CA
commented
Priority: Normal » Critical
nancydru’s picture

nancydru
she/her/hers
Primary language English
Location Boston
commented
Priority: Critical » Normal

I fail to see how any of this is breaking your site, so this is not a "critical" issue.

The comment will be removed before an "rc" release is done.

As for "full of issues," please point me to the corresponding Drupal coding standards that support these changes. While I agree that it would be great if HTML allowed </div class="whatever">, that is not the case today. We already have users reporting memory problems, so the developers must consider those issues (which may very well be critical) as well.

When I have a few free minutes, I will try to find the SEO newsletter I received that suggests that <hx class="title"> is bad for search engine optimization. That article suggests that proper CSS does not need that.

kmonty’s picture

kmonty
He/Him
Location Berlin, DE + SF, CA
commented
Priority: Normal » Critical

I completely disagree with the priority. If performance or resources is a legitimate concern, than trying to deliver dozens of print_rs of the vars array into the page source is a huge problem! I was wondering why my page source was about 100k more than it should have been, which is why I filed this as critical. If you want to leave it in there for testing reasons, you should comment it out with a <?php /* */ ?> and just remove the php as needed.

I would argue the class="title" should be there as it is commonplace in Drupal themes and many themers attach different styles for h2.title and h2 (assuming h2.title is code generated (thus appears as the node title) and h2s are user generated (and therefore might have a special style as it appears to break up the section of an article). While this might not be true for Garland, I think we can both agree that most advanced users don't work with Garland :-)

I would also strongly suggest changing the IF logic as I suggested; it makes for less empty DIVs in the pagesource, which is nothing but a good thing!

nancydru’s picture

nancydru
she/her/hers
Primary language English
Location Boston
commented
Title: weblinks-link.tpl.php full of issues » weblinks-link.tpl.php issues

Funny, I was just running all this stuff through the HTML Validator and discovered my faux pas in the debugging code. I will definitely fix that. I also discovered another potentially serious issue there, so I will have to create a new release when I commit it.

most advanced users don't work with Garland It has nothing to do with the designer's level of expertise - the theme choice is up to the site owner. I have several live sites where the owner wanted the ease of changing colors; until more themes provide that, Garland will remain popular.

nancydru’s picture

nancydru
she/her/hers
Primary language English
Location Boston
commented
Status: Active » Fixed

Committed

Status: Fixed » Closed (fixed)

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