GHOP #108: Modify D6 FAQ module to use theme templates

Sorry for the delay in responding, I've been internet-less since Christmas.

So I've done a quick test of the patch and the initial feedback is below. However I did find one bug in the original checked-in code! There was a missing closing " for a class name which broke the faq node links in the questions inline mode. I've checked in the fix and attached a patch for your version.

My initial tests just involved testing of (a) the functionality and (b) the layout, i.e. basically that it behaves and looks the same as the previous version.

• faq-new-page.tpl.php: there should be no <h4>...</h4> tags in this file as it messes up the font size.
• faq-hide-answer.tpl.php: there is an extra <hr> tag on line 18 which shouldn't be present.
• faq-questions-inline.tpl.php: there are extra <strong>...</strong> tags around the question text which cause it to display incorrectly.
• faq-questions-top.tpl.php: there is an extra <hr> tag on line 12.

I'll conduct a more in-depth set of tests next. However, it's looking great! Thanks a mill!

Cheers,
Stella

Hi,

I've had another look at the patch, at the code this time. I quite like what you've implemented with the different variables available to the themer in the template file, especially the way that they're clearly documented. However, I would prefer if all of the html divs could be moved out of the module file and into the template files.

Here's an example:

      $nodes[$count]['link'] = '<div class="faq_question faq_dt_hide_answer">';
      $nodes[$count]['link'] .= l($node->title, "node/$node->nid") ."</div>\n";
      // should we display teaser or full text
      if ($display_vars['use_teaser']) {
        $more_link = '';
        if (!empty($display_vars['more_link']) && strlen($node->teaser) < strlen($node->body)) {
          $more_link = '<p class="faq_more_link">';
          $more_link .= l(t($display_vars['more_link']), "node/$node->nid") .'</p>';
        }
        $nodes[$count]['body'] = '<div class="faq_answer faq_dd_hide_answer">';
        $nodes[$count]['body'] .= check_markup($node->teaser, $node->format, FALSE);
        $nodes[$count]['body'] .= $more_link ."</div>\n";
      }

      // full text
      else {
        $nodes[$count]['body'] = '<div class="faq_answer faq_dd_hide_answer">';
        $nodes[$count]['body'] .= check_markup($node->body, $node->format, FALSE) ."</div>\n";
      }

could change to something like:

      $nodes[$count]['link'] = l($node->title, "node/$node->nid");
      // should we display teaser or full text
      if ($display_vars['use_teaser']) {
        $more_link = '';
        if (!empty($display_vars['more_link']) && strlen($node->teaser) < strlen($node->body)) {
          $more_link = l(t($display_vars['more_link']), "node/$node->nid");
        }
        $nodes[$count]['body'] = check_markup($node->teaser, $node->format, FALSE);
        $nodes[$count]['more_link'] = $more_link;
      }

      // full text
      else {
        $nodes[$count]['body'] = check_markup($node->body, $node->format, FALSE);
      }  

See the way all of the <div> tags have been removed. It could be that the themer doesn't want to use div tags around the answer text and instead wants to use <p> tags instead. You will need to modify the template files as appropriate. It will probably involve the use of a foreach loop but I think that's an acceptable amount of php code in a template file. See some of the forum module's template files for examples.

Cheers,
Stella

dmitrig01: answers to some questions below:

Q.1, good point, though I'm not sure that it's done that way in the forum module, maybe we need to add a minor bug report for that, or add a check to the coder module so it can check for it in future?

Q.3, again good point, I think it was probably inherited from the original version of the code, but the variable name should be made clearer.

Q.5, users can choose between ordered lists (<ol>) and unordered lists (<ul>). The $list_style variable probably contains ul or ol.

Q.7, display_vars stuff is inherited from the original version of code. I don't think modifying this should fall into the scope of this GHOP task.

Q.8, theme_faq_category_questions_top isn't a template because we haven't gotten that far yet. I thought it was too many functions to port to a template for one GHOP task. As soon as this task is completed, I'll open a 2nd task that ports this function, and others, to templates.

Cheers,
Stella

Hi,

Did another review of the patch. It's almost there. The only things left are minor layout and coding standards changes.

faq-new-page.tpl.php
Perfect!

faq-hide-answer.tpl.php

• The more link should be printed within the <div class="faq_answer faq_dd_hide_answer">...</div>.
• Some of the indentation in the corresponding preprocess function is incorrect, lines 1147 and 1153 (tabs are used instead of spaces).
• Line 21 of the template file, change this line to use !empty($node['more_link']) function rather than checking $node['more_link'] != ''.

faq-questions-inline.tpl.php

• Both the "more" and "back to top" links should be printed within the <div class="faq_answer">...</div>.
• The answer label should be within a <strong>...</strong> pair of tags in order to match the original layout.
• Line 27 of the template file, change it use to use php's empty() function instead of isset().
• Indentation on line 30 of the template file uses tabs instead of spaces.

faq-questions-top.tpl.php

• Line 874, in the corresponding preprocess function, breaks the Drupal coding standards - it uses a tab instead of a spaces.
• The question text that is printed just above the answer (i.e. not the ones in the list) is contained within the <div class="faq_answer"> section instead of it's own <div class="faq_question"> section.
• Again both the "more" and "back to top" links should be printed within the <div class="faq_answer">...</div> section.
• I might also suggest that you use <br /> or something similar just to add a wee bit of blank space between the question list and the Q&A text further down. This is only a minor thing though.

Here's what the html structure of the resulting faq page should follow:

<div class="faq">
  <div class="item-list">
    <ul class="faq_ul_questions_top">
      .........
    </ul>
  </div>
  <div class="faq_question"><a href="/node/126" name="n126">How do I edit my personal details?</a></div>
  <div class="faq_answer">
    <p>abcde</p>
    <p class="faq_top_link"><a href="/faq" class="active">Back to Top</a></p>
  </div>
  <div class="faq_question">............</div>
  .........
</div>

Each of the template files also need to have the CVS keyword $Id$ included in each file, e.g. /* $Id$ */.

You're pretty much there at this point. There's only a few minor changes left to make at this point. Well done!

Cheers,
Stella

Yep, the issue dmitrig01 identified above is the only outstanding issue! Everything else is perfect.

Cheers,
Stella

Perfect! It's great! Thanks for all your hard work. I really appreciate it. Good luck with your other GHOP tasks.

aclight, dmitrig01: would one of you mind updating the Google issue: Issue 108.

Cheers,
Stella

All checked in! Thanks again.

Cheers,
Stella

Released in FAQ 6.x-1.3.

Cheers,
Stella